Repository navigation
fix(orionbelt): name fields by column, quote only when needed, export OSSIE_SQL_2026 - #605
Conversation
… OSSIE_SQL_2026 The export named each Ossie field by its OBML column code. After an Ossie import that code can hold a calculated field's SQL, so amount_with_tax (amount * 1.1) came back as a field named "amount * 1.1" (apache#601). Fields are now named by the OBML column name with the code as their expression, and primary_key, unique keys and relationship columns use the same names. Documents from earlier versions still import; the unique keys and preserved metrics they carry are renamed with the fields. Metric references were always double-quoted, so "sales"."amount" did not match a field declared amount under ANSI identifier rules. A name is now quoted only when it cannot be a regular identifier or differs from another only by case. Each exported metric also carries an OSSIE_SQL_2026 expression where it has a form in that language, checked against core-spec/expression_language.md, so a metric imported in the spec's language keeps its tag (apache#576). An expression containing a query is preserved verbatim instead of having the subquery's aggregates lifted into measures. Adds sqlglot as a runtime dependency, as eight other converters do.
|
|
||
| field: dict[str, Any] = { | ||
| "name": code, | ||
| "name": col_name, |
There was a problem hiding this comment.
Naming fields by column name here breaks the ontology export, which this PR doesn't touch. OBMLtoOssieOntology still builds its mapping expressions from the table code and column codes (_table_ref, and _col_code in ontology.py). For a data object Orders with the primary key column Order ID (code order_id), it emits orders.order_id, but the embedded semantic model now has the field Order ID. The ontology spec defines mappings as SQL expressions over dataset fields, so none of the object or link mappings resolve any more. They all resolve on main.
I think ontology.py should build these expressions from the dataset name and the column name, quoted with ossie_ident, e.g. Orders."Order ID".
There was a problem hiding this comment.
The rename also changes dimension types in a round trip. On export, _extract_dimensions takes the dimension resultType straight from _infer_obml_type, a name heuristic that only matches keywords at _ boundaries.
When fields were named by code, unit_price matches price. Unit Price matches nothing, so OBML -> Ossie -> OBML turns a float dimension into resultType: string, and the same happens to Item Count (int). The column keeps its type because _convert_dataset checks datatype first.
Could the dimension use the same datatype-first lookup as the column? Moving those lines into a helper would cover both.
| for entry in (metric.get("expression") or {}).get("dialects") or []: | ||
| dialect = entry.get("dialect") |
There was a problem hiding this comment.
This assumes expression is a dict and that each dialect entry is a dict. Preserved metrics are exactly the ones the converter could not handle, so they are the most likely to be malformed. An older (unmarked) obml_unconverted_metrics payload holding {"name": "m", "expression": "SUM(Orders.amount)"} now makes OBMLtoOssie.convert() raise AttributeError: 'str' object has no attribute 'get'. On main it passes through.
Going the other way, Ossie -> OBML, a legacy document with obml_column_name stashes and such a metric crashes the same way. The crash goes through the loop at ossie_to_obml.py, which also needs an isinstance(metric, dict) check.
| for entry in (metric.get("expression") or {}).get("dialects") or []: | |
| dialect = entry.get("dialect") | |
| expression = metric.get("expression") | |
| if not isinstance(expression, dict): | |
| return | |
| for entry in expression.get("dialects") or []: | |
| if not isinstance(entry, dict): | |
| continue | |
| dialect = entry.get("dialect") |
| such as ``Order``) is quoted, and matches the declared name exactly, as is | ||
| a name in *collisions*, which bare would match another one. | ||
| """ | ||
| return sql_ident(name) if name in collisions or needs_quoting(name) else name |
There was a problem hiding this comment.
Quoting names that differ only by case (Orders."amount" vs Orders."Amount") relies on a quoted name matching exactly, and the rename_sql docstring says that's how the import resolves metric references. It doesn't: _resolve_column_refs strips the quotes and lower-cases the column, and fields_lc keeps only the first field for each lower-cased name (setdefault).
With fields Amount (expression amt) and amount (amt2), the metric SUM(Orders."amount") becomes a measure on Amount. The round trip comes back as SUM(Orders."Amount") and sums the wrong column without a warning.
The import should match a quoted column exactly and ignore case only for a bare one. A bare reference that matches two fields when case is ignored should be kept as an unconverted metric rather than resolved to the first field.
Related, and maybe worth settling first: if I read the spec's identifier rules right, an unquoted identifier is normalized to upper case, so a quoted "amount" would not match a field declared as amount either. That would change how these names should be written in the first place.
| try: | ||
| tree = sqlglot.parse_one(sql, read=_OSSIE_DIALECT_TO_SQLGLOT.get(dialect)) | ||
| except sqlglot.errors.SqlglotError: | ||
| return False | ||
| return tree.find(exp.Query) is not None |
There was a problem hiding this comment.
When sqlglot can't parse the text, this returns False, and unlike _field_names._parse it doesn't retry with [name] brackets. So a bracket-quoted metric with a subquery still gets the subquery's aggregates lifted, which is what this check is meant to stop.
For example, SUM([Orders].[amount]) + (SELECT MAX([Orders].[amount]) FROM adjustments AS [Orders]) produces a _Orders_amount_max measure and the broken metric {[_Orders_amount_sum]} + (SELECT {[_Orders_amount_max]} FROM adjustments AS [Orders]).
Reusing _parse fixes this; it would need to be imported, or made public:
| try: | |
| tree = sqlglot.parse_one(sql, read=_OSSIE_DIALECT_TO_SQLGLOT.get(dialect)) | |
| except sqlglot.errors.SqlglotError: | |
| return False | |
| return tree.find(exp.Query) is not None | |
| parsed = _parse(sql, _OSSIE_DIALECT_TO_SQLGLOT.get(dialect)) | |
| return parsed is not None and parsed[1].find(exp.Query) is not None |
…p ontology over fields Address the review of apache#605: - The ontology export builds its object and link mappings from the dataset and field names, quoted as the model's metrics are, so they resolve against the embedded semantic model (`Orders."Order ID"`, not `orders.order_id`). - A dimension takes its type from the field `datatype` first, like the column, instead of a guess from its name; a time dimension over a non-temporal field stays a date dimension. - Renaming a preserved metric skips an expression or dialect entry that is not a mapping, instead of raising AttributeError. - The subquery check retries a bracket-quoted expression as the rename does, so a bracketed metric with a subquery is preserved verbatim. - References follow core-spec/expression_language.md: a regular identifier is normalized to upper case, a quoted one is taken as written. The import matches a quoted reference exactly, else by its normalized form, and a bare one regardless of case; a reference that matches no field or more than one keeps the metric verbatim instead of resolving to the first. The export quotes a regular name DuckDB or sqlglot will not read bare in its normalized form ("QUALIFY"), and warns of names that differ only by case.
…nsions by datatype (#599) Mirror the review of apache/ossie#605 in the in-repo converter: - References follow the Ossie expression language's identifier rules: a bare name is normalized to upper case, a quoted one is taken as written. The import matches a quoted reference exactly, else by its normalized form, and a bare one regardless of case; a reference that matches no dataset or field, or more than one, keeps the metric verbatim instead of resolving to the first. Databricks compares quoted names regardless of case, so there they are read as bare. The export quotes a regular name DuckDB or sqlglot will not read bare in its normalized form ("QUALIFY") and warns of names that differ only by case. - An imported dimension takes its type from the field datatype first, like the column; a time dimension over a non-temporal field stays a date one. - Renaming a preserved metric skips an expression or dialect entry that is not a mapping, instead of raising AttributeError.
kayemkim
left a comment
There was a problem hiding this comment.
On a document the earlier export wrote, _restore_column_names renames the column keys, each dimension's column, the measure references, the unique keys and the preserved metrics, but not the dimension names themselves. Those come out of _extract_dimensions as the field name, which in such a document is the code or the expression. obml-to-ossie on main from tests/fixtures/tpcds_as_obml.yaml, then ossie-to-obml on this branch, gives dimensions a key c_first_name || ' ' || c_last_name with column: customer_full_name, where the fixture names that dimension customer_full_name. The same with a _legacy_field-shaped input: a field named amount * 1.1 that stashes Amount With Tax and carries dimension comes back with the column key Amount With Tax and the dimension key amount * 1.1, with no warning. Renaming the dimension keys in that pass would close it, and TestLegacyCodeNamedExport has no dimension assertion yet. Everything else in that cross-version trip matches the fixture: columns, codes, keys, joins, measures and metrics.
On d40a0a6, the M1 fixture keeps both field names, the TPC-DS example keeps 31 of 31 and examples/flights.semantic_model.yaml 51 of 51. Every metric in the eight #596 fixtures comes back with its text unchanged and the second dialect added, the OSSIE_SQL_2026 tag on m0_ossiesql_qualified survives, and each returned document passes validate.py. The bare-form fixtures still gain the helper measures they did on main, which this PR does not touch. The five points from the review behave as the second commit says with my inputs: Orders."Order ID" in the object mapping, Unit Price back as float and Item Count as int, a string-valued preserved metric passing through in both directions, SUM(Orders."amount") next to Amount resolving to amount while the bare reference is kept and warned, and the bracket-quoted subquery kept verbatim.
On the identifier question. As the branch stands, a declared regular name goes out bare and spelled as declared (sales.TotalAmount), so a reader that matches exactly and one that folds case both resolve it, and the import takes "TOTALAMOUNT", "TotalAmount" and a bare reference in any case. A reserved word (value, order, date, count) or a name that cannot be regular (Order ID, 2nd_col) goes out quoted as written, and Qualify as "QUALIFY". The one case where this and a validator could disagree is a declared name that is a reserved word: this branch treats it as delimited, while a validator that normalizes declared names by shape alone would upper-case it to VALUE and not match "value". That is the sentence the spec would need, and it does not block this PR.
jbonofre
left a comment
There was a problem hiding this comment.
LGTM, thanks 👍
The ontology mappings, the datatype-first dimension types, the malformed preserved-metric handling, the exact quoted-name matching and the bracket-quoted subquery check all behave as intended now.
We could consider the follow-ups:
_restore_column_namesrenames column keys, dimensioncolumn, measure references, unique keys and preserved metrics, but not the dimension keys themselves. A document written by the previous version can still come back with a dimension named by its code or expression.TestLegacyCodeNamedExportcould get a dimension assertion alongside the fix._convert_columnno longer falls back to the column name asdescriptionor adds the display-name synonym._extract_dimensionsnow forcesdatefor anis_timedimension over a non-temporal field. Both are reasonable, but they aren't listed in the description.
Summary
Three fixes to the OrionBelt converter, measured with the #596 portability fixtures and the TPC-DS example.
1. A field keeps its name (#601). The export named each Ossie field by its OBML column
code. After an Ossie import, the code of a calculated field is its SQL, soamount_with_tax(amount * 1.1) came back as a field namedamount * 1.1. Fields are now named by the OBML column name, with the code as their expression, which is the distinction an Ossie field already makes.primary_key,unique_keysand relationship columns use the same names.Documents written by earlier versions, which named fields by code and kept the column name in
obml_column_name, still import. The unique keys and preserved metrics such a document carries are renamed with the fields. Only the field references in a metric's SQL change; a table or alias the SQL declares itself is left alone, and a reference that cannot be renamed safely is kept and warned about.2. A reference is quoted only when it has to be (#576). Every metric reference was double-quoted. The expression language follows ANSI identifier rules (
core-spec/expression_language.md, "Namespacing and Identifier Resolution"): a regular identifier is normalized to upper case, a quoted one is taken as written, so"sales"."amount"did not match a field declaredamount. A name is now bare when it can be a regular identifier. A name that cannot (a space, a leading digit, a SQL:2023 reserved word such asOrderorvalue) is quoted as written. A regular name DuckDB or sqlglot will not read bare, such asQualify, is quoted in its normalized form,"QUALIFY", which matches it. Names that differ only by case are one identifier to the language; they are quoted as written, with a warning. The import resolves references by the same rules: a quoted reference exactly, else by its normalized form, a bare one regardless of case, and a metric whose reference matches no field or more than one is kept verbatim rather than resolved to the first. The ontology export's object and link mappings are written the same way, over the embedded model's dataset and field names.3. Metrics also carry
OSSIE_SQL_2026(#576). The export wrote every expression asANSI_SQL, so a metric imported in the spec's own language came back retagged. Each measure or metric now also carries anOSSIE_SQL_2026expression, in that language's forms (CONTAINS,STARTSWITH,ENDSWITHinstead ofLIKE ... ESCAPE, aCASTfor a timestamp literal). It is checked againstcore-spec/expression_language.mdboth as written (every function,CASTtype and typed literal is one the spec lists at the REQUIRED or RECOMMENDED level, and there is no foreign operator such as::) and as parsed (no construct the spec does not list). A measure with no such form (MODE, a pattern that needsESCAPE, an authored expression outside the language) keepsANSI_SQLonly. A function sqlglot respells (VARIANCE_POP,STR_POSITION,CAST(x AS TEXT(2))) is written in the spec's spelling (VAR_POP,CHARINDEX,CAST(x AS STRING(2))), keeping a type's precision or length.Also: an expression containing a query (a subquery or a
UNION) is now preserved verbatim. Before, the aggregates inside the subquery were lifted into measures, even where the subquery's own alias shadowed a dataset name.Round trip, Ossie to OBML to Ossie
mainm1_scalar_fieldamount_with_taxcomes back namedamount * 1.1customer_full_namecomes back namedc_first_name || ' ' || c_last_namem0_ossiesql_qualifiedANSI_SQL, textSUM("sales"."amount")OSSIE_SQL_2026kept, text unchanged"sales"."amount"On every fixture, each metric's text comes back unchanged and gains the other dialect. All nine returned documents pass
validation/validate.py.Dependency
This adds
sqlglot>=30,<31as a runtime dependency. Eight other converters already declare sqlglot (holistics, nvidia, snowflake, microsoft, dbt, cube, sigma, thoughtspot). The cap below 31 is deliberate: theOSSIE_SQL_2026check depends on sqlglot's token types and on how it spells normalized functions.Origin
These fixes shipped in OrionBelt's in-repo copy of this converter first (ralforion/orionbelt-semantic-layer#586, #587, #592, #594), where they were reviewed. The port is adapted to this copy: the import keeps its own metric parsing, and nested data objects, which this copy's OBML schema does not have, are left out.
Related Issues
Closes #601. Part of #576.
Checklist
Converters
converters/is updatedconverters/orionbelt/tests/: newtest_ossie_field_names.pyandtest_ossie_sql_2026_dialect.py, plus cases intest_ossie_portable_expressions.pyandtest_ossie_metric_no_silent_loss.pyDocumentation
converters/orionbelt/README.mdandossie_obml_mapping_analysis.mdupdatedTests
pytestinconverters/orionbelt: 625 passed, 5 skipped (the skips needduckdb, as before)ruff check,ruff format --checkandmypyreport nothing new againstmainCompliance
dev/check-licensepasses)