Skip to content

fix(validation): classify warnings independently of message text - #373

Open
flyrain wants to merge 2 commits into
apache:mainfrom
flyrain:yf/fix-validator-warning-classification
Open

flyrain wants to merge 2 commits into
apache:mainfrom
flyrain:yf/fix-validator-warning-classification

Conversation

@flyrain

@flyrain flyrain commented Sep 9, 2026

Copy link
Copy Markdown

This fix marks warnings explicitly by using a typed error instead of a text prefix, so user-provided text cannot disguise errors as warnings. It adds regression tests and includes SQLGlot in CI to cover SQL errors.

Validation

Seven regression cases failed before the fix and pass afterward. All 54 validator tests pass locally on Python 3.14, and the canonical example validates successfully:

  • uv run validation/test_validate.py — 34 passed
  • uv run --with pytest --with pyyaml --with jsonschema --with sqlglot -m pytest validation/tests/ — 20 passed
  • uv run validation/validate.py examples/tpcds_semantic_model.yaml — passed
  • git diff --check — passed

Checklist

Validation

  • New validation cases are covered by tests

Tests

  • All existing validator tests pass locally
  • New functionality is covered by tests

Compliance

  • ASF license headers are preserved
  • Only existing third-party dependencies are used

Copilot AI lite review requested due to automatic review settings September 9, 2026 23:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Updates the validator so warning vs error severity is determined by an explicit warning type rather than by user-controlled message text, preventing false “PASSED” results when inputs contain Warning:.

Changes:

  • Introduce a ValidationWarning string subtype and classify diagnostics by type instead of message content.
  • Convert existing warning-producing paths (reference key-coverage, missing-sqlglot) to emit ValidationWarning.
  • Add regression tests for warning-like names/text and update CI to install sqlglot so SQL validation tests run.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
validation/validate.py Adds ValidationWarning and updates warning/error classification to be type-based.
validation/tests/test_validate.py Adds regression tests and a helper fixture to exercise CLI exit codes/output.
.github/workflows/validation-ci.yml Installs sqlglot in the pytest job to run SQL validation regressions.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread validation/tests/test_validate.py

@kayemkim kayemkim left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran this locally against a merge with current main (28365cd) on Python 3.11 through 3.14, following the steps in validation-ci.yml: the duplicate-key tests, the pytest suite with sqlglot, and the canonical example. All green, and the sqlglot test executed on every version rather than being skipped.

I also reproduced the bug on main. A relationship pointing at an unknown dataset named Warning: customers currently prints the reference error and then Validation PASSED with exit 0, because severity was decided by searching the message text. With this patch it fails with exit 1. The key-coverage warning from #330 (which I wrote) stays nonfatal, and the case where a genuine warning and a real error appear together still exits 1.

Validating every semantic-model YAML in the repo (examples and converter fixtures, 11 files) gives identical results before and after, so nothing that passes today changes status.

One small observation, not a request: the Warning: prefix in the two messages is now purely cosmetic, since severity comes from the type. Keeping it seems right for the humans reading the output.

LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants