Repository navigation
feat(core-spec): decouple core spec from specific vendors - #617
christianeu-db wants to merge 8 commits into
Conversation
3fbb2aa to
2885bd1
Compare
2885bd1 to
f60b217
Compare
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>
f60b217 to
f8d3508
Compare
kayemkim
left a comment
There was a problem hiding this comment.
I read this alongside the list thread and merged it locally onto current main (b42ce09), no conflicts. Three things on the code; the design points are on the list.
The hex check job fails at the head: ty check reports invalid-argument-type at load_ossie_dialect_expression.py:79, where _to_sqlglot_dialect(ossie_dialect: OssieDialect) now receives the str that OssieDialectExpression.dialect became. The runtime side matters more: that function ends in assert_never, so a document with a dialect outside the old enum, which this PR makes valid, raises AssertionError: Expected code to be unreachable, but got: 'DB_DIALECT' on export. I ran it directly with DB_DIALECT and SNOWFLKE; both raise. The wisdom test you added covers this case; hex needs the same, with the final branch returning None for anything unregistered and the parameter typed str.
Expression still lists dialects under required with minItems: 1, and OssieExpression.dialects keeps Field(..., min_length=1), so a portable-only document fails both the schema and the SDK:
[Schema] datasets -> 0 -> fields -> 0 -> expression: 'dialects' is a required property
A required of one of expression or dialects (anyOf) in the schema and a model validator in the SDK would match the behaviour the summary describes.
With #612 cherry-picked on top, the two tests that read $defs.Dialect.enum fail with KeyError (validation test_every_schema_dialect_is_mapped_or_skipped, Holistics test_every_schema_dialect_is_parsed_or_marked_not_sql); the table cross-check and the sqlglot reader tests pass. I can point those two at the table in extensions/dialects.md instead, in whichever order the two PRs land.
…from-vendor-dialects # Conflicts: # converters/dbt/src/ossie_dbt/ossie_to_msi.py # converters/hex/src/ossie_hex/ossie_to_hex/load_ossie_expression.py # converters/hex/src/ossie_hex/ossie_to_hex/pick_ossie_dialect_expression.py # converters/hex/tests/ossie_to_hex/test_pick_ossie_dialect_expression.py # converters/microsoft/tests/test_semantic_model_to_ossie.py # converters/omni/tests/_util.py # converters/ontology/src/ossie_ontology/converter/ossie_to_spec/converter.py # converters/ontology/src/ossie_ontology/converter/spec_to_ossie/converter.py # converters/ontology/src/ossie_ontology/model.py # converters/ontology/src/ossie_ontology/spec.py # converters/sigma/src/ossie_sigma/ossie_to_sigma.py # converters/wisdom/tests/test_ossie_to_wisdom.py # converters/wisdom/tests/test_wisdom_to_ossie.py
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>
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>
Brings in the merged apache#591 (dataset-scoped metrics), apache#486 (name resolution), apache#614 and apache#619. Resolved conflicts: - python/src/ossie/models.py: union the pydantic imports (model_validator + SerializationInfo + TypeAdapter). - core-spec/spec.yaml, core-spec/spec.md: keep both the portable expression/dialects presence note and the upstream name-resolution notes; they are complementary. - validation/validate.py (semantic, not a text conflict): the shared _parse_sql_expression from apache#591 returns a (tree, error) tuple, so the unknown-dialect skip must return (None, None), not a bare None. This also makes validate_metric_scoping treat an unknown dialect as opaque. Co-authored-by: Isaac <no-reply@databricks.com>
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>
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>
c1c223d to
c049fdb
Compare
Hi @kayemkim I've addressed the validator concern. The converter change will be addressed separately in ossie-converters. As I called out in the list, I decided to update the PR to incorporate your feedback around and expression iter (this had better ROI after merging in #591) |
`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>
c049fdb to
bd29692
Compare
abolfazlmadanii
left a comment
There was a problem hiding this comment.
Thanks for this, the top-level sql reads much better than burying the portable form as one more dialect entry. It also lines up with what I saw when measuring payloads for #616: on portable-only expressions you drop the dialects: wrapper and the dialect: line every time, which adds up fast in big models.
A few small things from reading the latest head:
-
Both forms present and different. If a doc has a top-level
sqland also anOSSIE_SQL_2026entry indialectswith different text, the validator checks both and says nothing. Since the spec says the top-level one wins, the other is effectively dead text. A warning when they differ (or even just when both exist) would catch drift early. -
Empty string. The new
sqlproperty has nominLength: 1, sosql: ""passes the schema and the SDK (Optional[str]), and only the SQL check catches it. AddingminLength: 1would keep the schema, the SDK and the validator in agreement. -
Converters. The hex
assert_nevercrash kayemkim found lives in ossie-converters now, so it's probably worth listing in the converter follow-up issue so it doesn't get lost when dialects go free-form. -
Nice to have. The unknown-dialect warning could suggest the closest known name (
difflib.get_close_matchesagainstDIALECT_MAP), soSNOWFLKEsays "did you mean SNOWFLAKE?".
None of these block the direction, which I'm +1 on.
|
Re-ran the same checks on bd29692 merged onto current main (7205e15): validation 261 passed, python 217, the four workflows CI would run here all pass locally, spec.yaml in sync, and the examples come out as on main. Of the three things above, the portable-only document now passes both the schema and the SDK, and the hex crash left with the converters, so abolfazlmadanii's note to carry it into the ossie-converters issue covers it. #612 still fails on top of this branch, now for two reasons: the enum read and the DIALECT_MAP rename. No need to repoint it on your side. I will rebase #612 after this lands and pin SQLGLOT_DIALECTS and NON_SQL_DIALECTS to the table in extensions/dialects.md, which today lists exactly the same twelve tokens. #595 and #608 move onto iter_expressions at the same time. Two small things from probing the new head. |
Summary
Context
Discussion for this PR is in this thread: https://lists.apache.org/thread.html/vmgv5ffryw70wxbvtwnohhhz2zzx0ocf
Ossie is a vendor-neutral specification. Its extensibility model already reflects
this: a
custom_extensionsentry pairs a free-formvendor_namewith a singleopaque
datastring. A vendor or an individual can attach any amount of extrastate, but that state is cordoned into one opaque value rather than promoted to
top-level spec fields. The core stays small and vendor-neutral, and everything
vendor-specific is opaque payload that the core carries but does not interpret.
Expressions are the one place where this principle is not yet applied. This PR
extends the same pattern to expressions and dialects. It is a spec-level change;
see the mailing-list threads under Related Issues.
1. Make the portable Ossie expression the primary field (backward-compatible)
Today an expression is a list of
dialects, each a{dialect, expression}pair.Ossie defines one portable dialect,
OSSIE_SQL_2026(
core-spec/expression_language.md), but in practice it sits as just anotherentry in the
dialectslist, peer to vendor dialects.This is the only place in the spec where an Ossie concept and a vendor
concept appear at the same level, and the portable form is rarely authored as a
result (there are some workstreams in flight to close this gap).
This PR adds an optional top-level
sqlfield to the expression object. Itis the primary, portable
OSSIE_SQL_2026. Thedialectslistremains for vendor-specific variants. When both the top-level
sqland anOSSIE_SQL_2026entry indialectsare present, the top-level field isauthoritative. For portable-only expressions, this leads to a more concise representation
since the dialect sections / dialect type string can be dropped.
The field is optional and additive, so the change is backward-compatible: existing
documents remain valid.
Note: a prior iteration had the nesting as expression: expression: to stay consistent with the (dialect, expression) pair but the feedback on the thread from @jochenchrist pointed to the double-key nesting feeling wrong
2. Make
dialecta free-form string, not an enumdialectis currently a closed enum incore-spec/ossie-schema.json. Adding adialect requires a change to the core spec and a DISCUSS thread, and a dialect
outside the enum fails validation by default, so an outside party cannot register
its own dialect without a full spec change. Yet these dialects are explicitly
opaque: an implementation MAY ignore any non-
OSSIE_SQL_2026dialect, andregistering a dialect does not imply Ossie can query it.
vendor_nameis already a free-form string for exactly these reasons. This PRapplies the same decoupling to
dialect: enum to free-form string. One could arguedialects belong entirely in
custom_extensions, but expressions are fundamentalenough to keep as a first-class field; only the closed enumeration is removed.
Examples: expression portability before / after this PR
sqlmeans "not portable."FakeDBis not in the enum; valid only after editingcore-spec/spec.yamland regenerating the SDK types.extensions/dialects.md(docs only).Rows 1–3 are additive: every existing document stays valid and
dialectskeeps working. Row 4 is the new capability, mirroring how
vendor_namealready works for
custom_extensions.Why this matters
Together, the future shape is an expression whose primary, portable form is defined
once at the top level, with dialects as optional overrides. The Hex converter calls
out the variety of expression types: not every dialect is a SQL dialect - some are richer
expression languages. More broadly, there is no reason an expression could not be a
code snippet or any other evaluation detail. A closed enum is too restrictive for
that range; a free-form string is not.
Extensions registry
The dialect and vendor example constants move out of the normative spec into a new
top-level
extensions/folder (README.md,dialects.md,vendors.md). This is acommunity registry of well-known tokens: a place to align on their spelling and
meaning without making any token fundamental to the spec. Presence in a list does
not imply endorsement, and new tokens can be added without a full spec review.
Changes in this PR
core-spec/ossie-schema.json:Dialectbecomestype: stringwith illustrativeexamplesand noenum;Vendorexamples are genericized; theExpressionobject gains the optional (Ossie)
sqlfield with its precedence rule.core-spec/spec.md,spec.yaml,expression_language.md: document the free-formdialect, the portable
sqlfield, and the link toextensions/; theopaque / declaration-only text is retained.
extensions/: newREADME.md,dialects.md,vendors.md.scripts/generate-spec-types.py: no longer generates adialectsenumeration(dialects are free-form, like vendors); it still syncs
datatypes.python/SDK:OssieDialectExpression.dialectis typedstr;OssieExpressiongains the optional
sql;OssieDialect/OssieVendorremain asconvenience constants.
validation/validate.py: an unregistered dialect is treated as opaque and skipped (with a warning)rather than parsed as ANSI SQL; the portable
sqlis validated asOSSIE_SQL_2026.validation/validate.py: cleanup on validation to extract dialect logic as an iterator / incorporate feat(core-spec): support dataset-scoped metrics #591.examples/tpcds_semantic_model.yaml: demonstrates the portable ossiesqlfield.Scope and follow-ups
The scope is deliberately minimal. The following refinements are left for follow-up
PRs:
core — into the converters or a shared mixins package — rather than living
alongside the core models. This can be done incrementally.
OSSIE_SQL_2026reading logic should be generalized to bias toward the primarytop-level
sqlwhen it is defined. This PR documents that precedence in thespec and the reference validator honors it, but the converters are not yet updated
to read the field.
Related Issues
Checklist
Specification
core-spec/and follow the existing structuredialectfield type changed from an enum tostr)Ontology
ontology/are consistent with spec changes (N/A — no ontology changes)Converters
Validation
validation/are updated if the spec changedDocumentation
docs/is updated to reflect any user-facing changes (follow-up; see Scope)core-spec/andexamples/)CONTRIBUTING.mdis updated if the contribution process changed (N/A)Examples
examples/are added or updated for any new spec constructs or converter support. Examples in the spec and updated a store sampleTests
pytest/ CI green)Compliance
NOTICEand/orLICENSEhave been updated per ASF policy (N/A — no vendored third-party source)