test(validation): pin the precedence the NOT rank describes by ayushtkn · Pull Request #630 · apache/ossie

ayushtkn · 2026-10-22T16:53:53Z

Summary

Followup from: #614

Three expressions parsed through the validator's own OSSIE_SQL_2026 dialect entry: NOT a = b builds Not(EQ(a, b)), - a = b builds EQ(Neg(a), b), and NOT a AND b builds And(Not(a), b). Together they fix NOT between comparison and AND, and show that the unary operators it had been listed beside bind ahead of comparison instead.

Related Issues

Checklist

Specification

Spec changes are included in core-spec/ and follow the existing structure
Spec changes have been discussed on the mailing list or in a linked issue
Breaking changes to the spec are clearly called out in the summary

Ontology

Ontology changes in ontology/ are consistent with spec changes
New or modified terms are defined and documented

Converters

Spec or ontology changes that affect converters have a matching issue or PR in apache/ossie-converters

Validation

Validation rules in validation/ are updated if the spec changed
New validation cases are covered by tests

Documentation

docs/ is updated to reflect any user-facing changes
New features or behaviors are documented with examples where appropriate
CONTRIBUTING.md is updated if the contribution process changed

Examples

examples/ are added or updated for any new spec constructs

Tests

All existing tests pass (pytest / CI green)
New functionality is covered by tests

Compliance

ASF license headers are present on all new source files
No third-party dependencies are added without PMC/IPMC approval
If third-party source code is vendored/copied (not just declared as a dependency), NOTICE and/or LICENSE have been updated per ASF policy

Three expressions parsed through the validator's own OSSIE_SQL_2026 dialect entry: `NOT a = b` builds Not(EQ(a, b)), `- a = b` builds EQ(Neg(a), b), and `NOT a AND b` builds And(Not(a), b). Together they fix NOT between comparison and AND, and show that the unary operators it had been listed beside bind ahead of comparison instead.

Copilot

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.

Copilot review overview

1 open issue

_parse_ossie_sql is unconditionally defined and calls into _VALIDATE.sqlglot even when… · New

What changed in this PR

Adds validator tests to pin the operator precedence behavior of the OSSIE_SQL_2026 dialect, specifically for NOT, unary -, comparisons, and AND, as a follow-up to #614.

Changes:

Add a small parsing helper for OSSIE_SQL_2026 expressions used in tests.
Add three new precedence tests covering NOT a = b, - a = b, and NOT a AND b.

File	Description
validation/tests/test_validate.py	Adds new sqlglot-based tests to lock in operator precedence expectations for OSSIE_SQL_2026 parsing.

_{Review effort: Lite}

+def _parse_ossie_sql(expression: str):
+    """Parse *expression* the way the validator parses an OSSIE_SQL_2026 body."""
+    return _VALIDATE.sqlglot.parse_one(
+        expression, dialect=_VALIDATE.DIALECT_MAP["OSSIE_SQL_2026"]
+    )


Bring in apache#630 (precedence tests pinning the NOT rank), which landed on main after the previous merge. apache#630 adds three OSSIE_SQL_2026 precedence tests whose helper parses through `DIALECT_MAP["OSSIE_SQL_2026"]`. This branch split that map into `SQLGLOT_DIALECTS` (parseable) and `NON_SQL_DIALECTS`, so the merged-in helper is pointed at `SQLGLOT_DIALECTS[PORTABLE_DIALECT]`, which is the same None (sqlglot default) entry; the three tests pass unchanged otherwise. Verified against validation-ci.yml: duplicate-key tests (59), validation pytest (271, incl. the three apache#630 tests), python model tests (266), generator --check in sync, and all five example validations pass. Co-authored-by: Isaac <no-reply@databricks.com>

apache#630 added OSSIE_SQL_2026 precedence tests whose helper parses through `DIALECT_MAP["OSSIE_SQL_2026"]`. This branch split that map into `SQLGLOT_DIALECTS` (parseable) and `NON_SQL_DIALECTS`, so the merged-in helper hit `AttributeError: module 'ossie_validate' has no attribute 'DIALECT_MAP'`. Point it at `SQLGLOT_DIALECTS[PORTABLE_DIALECT]`, the same None (sqlglot default) entry, so the three tests pass unchanged. Co-authored-by: Isaac <no-reply@databricks.com>

@abolfazlmadanii

* feat(core-spec): decouple core spec from specific vendor dialects Make the core specification vendor-neutral: - `Dialect` becomes a free-form string (was a closed enum), mirroring the already-free-form `Vendor`; generic schema `examples` replace concrete product names. - Move the known-dialect and known-vendor catalogs out of the normative spec into a new top-level `extensions/` folder (README, dialects.md, vendors.md), framed as a community registry whose listings do not imply endorsement. - Add an optional top-level portable `expression` field (OSSIE_SQL_2026) to the expression object; when both it and an OSSIE_SQL_2026 `dialects` entry are present, the top-level field is authoritative. Demonstrated on `customer_full_name` in examples/tpcds_semantic_model.yaml. - Keep the normative text that non-OSSIE dialects are opaque and declaration-only. Kept in sync: the enum-sync generator no longer emits `dialects` (free-form, like vendors); the Python SDK types `dialect` as a free-form string and gains the optional portable `expression`; the reference validator treats unregistered dialects as opaque (skipped) and validates the portable expression. Converters are otherwise unchanged except where the now-string dialect required a fix (the dbt dialect comparison uses `==`, and the sigma and hex converters no longer call `.value` on a dialect) and where the new portable `expression` is handled: the ontology converter carries it through its model and both conversion directions, and omni's round-trip test documents the portable marker as a normalization (its text survives as the dialect entry). Co-authored-by: Isaac <no-reply@databricks.com> * fix(core-spec): allow portable-only expressions without dialects An Expression required `dialects` on both the JSON Schema and the SDK, so a document carrying only the portable top-level `expression` (OSSIE_SQL_2026) was rejected, contradicting the spec's stated semantics. - Schema: replace `required: [dialects]` with an anyOf requiring at least one of `expression` or `dialects`; `dialects` keeps `minItems: 1` when present. - SDK: make `OssieExpression.dialects` optional and add a model_validator that rejects an expression carrying neither form. - Tests: cover portable-only, dialects-only, both-present, and the empty cases on both the schema and the SDK. - Docs: note the "at least one" rule in spec.md and spec.yaml. Co-authored-by: Isaac <no-reply@databricks.com> * feat(validation): warn on unknown dialects instead of skipping silently An unrecognized dialect is opaque and legitimate under the free-form `Dialect` model, so its SQL cannot be parse-checked. The validator skipped it silently, which also hid typos such as `SNOWFLKE`. `validate_sql` now tallies expressions whose dialect is not in `DIALECT_MAP` and emits one nonfatal `ValidationWarning` per distinct unknown dialect, with the count of affected expressions. Validation still passes. Known-but-unparseable dialects (MDX, DAX, HOLISTICS_AQL, ...) stay silent, and the per-call `validate_sql_expression` is unchanged. Co-authored-by: Isaac <no-reply@databricks.com> * refactor(core-spec): rename portable expression key to 'sql' The portable top-level key of the Expression object was `expression`, nested under a field/metric's own `expression:` block -- the doubled `expression.expression` Jochen flagged on the thread. Rename it to `sql` so the key reads cleanly and names what it holds: the portable OSSIE_SQL (OSSIE_SQL_2026) form. Scope is the portable key only. The field/metric `expression:` key (which holds the Expression object) and the per-dialect `dialects[].expression` key (already qualified by its `dialect:`) are unchanged. This key was introduced by this same unreleased PR, so the rename carries no migration burden; no released spec ever had `expression.expression`. No back-compat alias -- the old key is rejected (schema additionalProperties:false, SDK extra=forbid). Touches the schema anyOf + property, the SDK model and validator, the validator's portable reads, spec.md/spec.yaml/docs prose and examples, the tpcds portable example, and the mirrored tests. Co-authored-by: Isaac <no-reply@databricks.com> * fix(validation): check a dataset-scoped metric's portable sql A dataset-scoped metric may be written with only the portable `sql` form (no dialects). Such a metric escaped both checks: `validate_sql` only walked its `dialects`, so the portable form was never parsed; and `validate_metric_scoping` likewise only walked `dialects`, so the single-namespace scoping rules (no qualified references, declared fields only) were never applied to it. Both now handle the portable `sql` exactly like a dialect expression: it is parsed as OSSIE_SQL_2026 and scope-checked. A portable-only dataset-scoped metric with a cross-dataset reference or an undeclared field is now reported, as a dialect form already was. Adds four tests covering the portable form: a declared single-part field passes, a cross-dataset reference and an undeclared field are rejected, and an invalid portable expression is reported. Co-authored-by: Isaac <no-reply@databricks.com> * refactor(validation): walk expressions through one iterator `validate_sql` and `validate_metric_scoping` each walked the document to find every expression. Introduce `iter_expressions`, the single traversal both consume, yielding an `Expression` record per portable `sql` form and per dialect expression, keyed by `kind` (field / dataset_metric / model_metric). Also folded in while here: - Split the overloaded DIALECT_MAP into SQLGLOT_DIALECTS (parseable, mapped to the sqlglot dialect) and NON_SQL_DIALECTS (carried but not parsed); one guard in _parse_sql_expression indexes SQLGLOT_DIALECTS. - Name the PORTABLE_DIALECT constant; drop the `.get("dialect", "ANSI_SQL")` fallback, since the schema requires `dialect`. - A dialect this validator cannot parse is reported as a nonfatal warning that its expressions were not syntax-checked; it may be a legitimate entry from extensions/dialects.md, not a mistake. - Honor the spec's precedence rule: a top-level `sql` outranks an OSSIE_SQL_2026 dialect entry, so that entry is non-authoritative. It is not scope-checked, and an invalid one warns instead of failing the document, since a client may still read it. - Spec/docs wording: write OSSIE_SQL_2026 directly (no parenthetical), "Any string is accepted", drop "Optional:" from the portable-form comments, describe `data` as an opaque string (not JSON), and give the glossary Dialect entry only ANSI_SQL as an example. - models.py: optional `dialects` comment and the "convenience set, not a closed one" phrasing removed; docstrings state any string is accepted. - Tests track the split, assert exact scope messages, cover the precedence behavior, and compare dialects by value. Co-authored-by: Isaac <no-reply@databricks.com> * fix(validation): tighten sql/dialect handling per PR #617 review Addresses review feedback on the decouple-core-spec PR (#617): - Require non-empty expression and identity strings: add minLength 1 to `sql`, `dialect`, the per-dialect `expression`, and `vendor_name` in both the schema and the SDK, so an empty value is rejected there rather than slipping through -- an empty `sql`/`expression` reached the SQL validator, an empty `dialect` produced an "is not supported" warning, and an empty `vendor_name` was unchecked. Suggested by @abolfazlmadanii. - Warn when a non-authoritative OSSIE_SQL_2026 dialect entry drifts from the authoritative top-level `sql`: a differing or invalid entry is dead text a client might still read, so report it as a nonfatal warning; an identical one stays silent. - Suggest the closest known dialect on an unsupported one (difflib, cutoff 0.8), so `SNOWFLKE` says "Did you mean 'SNOWFLAKE'?" without flagging a genuinely new dialect. - extensions/README.md: note that dialect and vendor tokens are matched exactly and are case-sensitive, so `snowflake` is distinct from `SNOWFLAKE`. - Describe custom-extension `data` as an opaque string, not a JSON string, in the schema and SDK, matching the spec/docs wording. Co-authored-by: Isaac <no-reply@databricks.com> * docs(extensions): register ATACCAMA in the vendor catalog Add the ATACCAMA token to the known-vendor table, per #629. Co-authored-by: Isaac <no-reply@databricks.com> * fix(ci): point #630's precedence tests at SQLGLOT_DIALECTS #630 added OSSIE_SQL_2026 precedence tests whose helper parses through `DIALECT_MAP["OSSIE_SQL_2026"]`. This branch split that map into `SQLGLOT_DIALECTS` (parseable) and `NON_SQL_DIALECTS`, so the merged-in helper hit `AttributeError: module 'ossie_validate' has no attribute 'DIALECT_MAP'`. Point it at `SQLGLOT_DIALECTS[PORTABLE_DIALECT]`, the same None (sqlglot default) entry, so the three tests pass unchanged. Co-authored-by: Isaac <no-reply@databricks.com> --------- Co-authored-by: Isaac <no-reply@databricks.com>

Copilot AI balanced review requested due to automatic review settings October 22, 2026 16:53

Copilot AI reviewed Oct 22, 2026

View reviewed changes

Comment thread validation/tests/test_validate.py

Comment on lines +378 to +382

def _parse_ossie_sql(expression: str):

"""Parse *expression* the way the validator parses an OSSIE_SQL_2026 body."""

return _VALIDATE.sqlglot.parse_one(

expression, dialect=_VALIDATE.DIALECT_MAP["OSSIE_SQL_2026"]

)

Copilot stopped reviewing on behalf of ayushtkn due to an error October 22, 2026 20:13
The job was not acquired by a runner even after multiple attempts.

github-actions Bot added the validation label Oct 22, 2026

christianeu-db approved these changes Oct 23, 2026

View reviewed changes

christianeu-db merged commit 01a5488 into apache:main Oct 23, 2026
9 checks passed

Navigation Menu

Repository navigation

test(validation): pin the precedence the NOT rank describes - #630

test(validation): pin the precedence the NOT rank describes#630
christianeu-db merged 1 commit into
apache:mainfrom
ayushtkn:test/not-precedence

ayushtkn commented Oct 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

ayushtkn commented Oct 22, 2026

Summary

Related Issues

Checklist

Specification

Ontology

Converters

Validation

Documentation

Examples

Tests

Compliance

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants