Skip to content

fix(dbt): read OSSIE_SQL_2026 expressions - #464

Merged
jbonofre merged 2 commits into
apache:mainfrom
RyutoYoda:fix/dbt-ossie-sql-2026
Sep 25, 2026
Merged

jbonofre merged 2 commits into
apache:mainfrom
RyutoYoda:fix/dbt-ossie-sql-2026

Conversation

@RyutoYoda

Copy link
Copy Markdown
Contributor

Summary

#439 added OSSIE_SQL_2026 to the core spec and #440 added it to the Python
OssieDialect enum. The dbt converter was never updated — OSSIE_SQL_2026 appears
nowhere in converters/dbt.

_get_expression() matches only self._dialect, which is always ANSI_SQL because
the constructor argument is never passed (cli.py:80, and every test), and otherwise
returns dialects[0]. Expression.dialects carries no ordering constraint, so an
expression written in OSSIE_SQL_2026 alongside a vendor dialect resolves by array
position:

  • [OSSIE_SQL_2026, SNOWFLAKE] gives the metric expr amount
  • [SNOWFLAKE, OSSIE_SQL_2026] gives amt_snowflake_only

In the second case a Snowflake-only column reference is written into the MetricFlow
output as portable SQL, with no warning and no ConverterIssue.
expression_language.md asks implementations to always support the Ossie dialect and
to choose between dialects deterministically; this did neither.

No bundled example carries more than one dialect per expression, so this is not
reachable from examples/; it needs a hand-written or third-party document, which the
schema permits (dialects is an ordered array with no uniqueness or ordering
constraint).

OSSIE_SQL_2026 is ANSI-SQL-compatible, so this adds it to the chain:
self._dialect, then OSSIE_SQL_2026, then dialects[0] — the order #443
established for orionbelt. Expressions carrying ANSI_SQL keep their current
behaviour, which is why the 113 existing tests are unchanged.

msi_to_ossie.py writes a single dialect on the way out (ANSI_SQL by default), so
the reverse direction is untouched.

On the precedence order

expression_language.md also proposes making OSSIE_SQL_2026 the default when no
dialect is chosen, which would argue for preferring it over ANSI_SQL. That is a
behaviour change for existing documents, so this PR leaves it as a future one-line
reordering.

This is a partial fix. #461 lists two positional reads; this one covers
_get_expression(). The bare-column scan at ossie_to_msi.py:401, which compares
dialects[0] and falls through to datasets[0], is left for a follow-up so this
change stays on one selection path.

#437 refactors this same function into _get_expression() / _get_dialect_expression().
It is currently in conflict with main and has changes requested, so this PR targets
main as it stands; I am happy to rebase onto whichever lands first.

Verification:

  • 119 tests pass on Python 3.11, 3.12, 3.13 and 3.14 (113 existing, unchanged, plus 6 new).
  • The two tests that target the defect fail on main; the other four assert preserved
    behaviour and pass either way.
  • Reproduced end-to-end through ossie-dbt ossie-to-msi on two documents differing only
    in dialect order, against main and against this branch.
  • Coverage of ossie_to_msi.py goes from 97% to 98% — the dialects[0] fallback was
    previously unreached, because tests/helpers.py only builds single-dialect expressions.

Related Issues

Partial fix for #461.

Checklist

Sections that this change does not touch (Specification, Ontology, Validation,
Documentation, Examples) are left unchecked.

Converters

  • Converter logic in converters/ is updated to reflect spec or ontology changes
  • New converters include tests under the converter's test directory

Tests

  • All existing tests pass: converters/dbt 119 passed on Python 3.11-3.14
  • New functionality is covered by tests: six regression tests, and the two targeting the defect fail without the fix

Compliance

  • ASF license headers are present on all new source files (no new files)
  • No third-party dependencies are added

OSSIE_SQL_2026 (apache#439, apache#440) was not recognised anywhere in converters/dbt.
_get_expression() matched only self._dialect -- always ANSI_SQL, since the
constructor argument is never passed -- and otherwise returned dialects[0].
Expression.dialects carries no ordering constraint, so an expression written
in OSSIE_SQL_2026 alongside a vendor dialect resolved by array position: with
the vendor entry first a Snowflake-only column reference was written into the
MetricFlow output as portable SQL, with no warning and no ConverterIssue.
expression_language.md asks implementations to always support the Ossie
dialect and to choose deterministically between dialects; this did neither.

Add OSSIE_SQL_2026 to the chain as an ANSI_SQL equivalent (self._dialect >
OSSIE_SQL_2026 > dialects[0]), mirroring apache#443/apache#446/apache#447 for orionbelt,
databricks and snowflake. The change is additive: expressions carrying
ANSI_SQL keep their current behaviour.

Add regression tests for the metric and field paths in either dialect order,
for ANSI_SQL keeping precedence over OSSIE_SQL_2026, and for the positional
fallback when no portable dialect is present. tests/helpers.py only builds
single-dialect expressions, so this is the first multi-dialect coverage in
the Ossie -> dbt direction.

Partial fix for apache#461 (_get_expression only); the positional read at
ossie_to_msi.py:401 is left for a follow-up.
jbonofre
jbonofre previously approved these changes Sep 25, 2026
if dialect_expr.dialect is self._dialect:
return dialect_expr.expression
if dialect_expr.dialect is OssieDialect.OSSIE_SQL_2026:
ossie_sql_expr = dialect_expr.expression

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe this new OSSIE_SQL_2026 preference isn't mirrored in _find_dataset_for_col(), which still reads field.expression.dialects[0].expression positionally and isn't touched by this PR. So the two functions can now resolve the same field to different text.

Your call if you want to fix it or not now 😄

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7d2b73d — _find_dataset_for_col now goes through _get_expression. Test added where the positional read attributes the metric to the wrong dataset.

for dialect_expr in ossie_expr.dialects:
if dialect_expr.dialect is self._dialect:
return dialect_expr.expression
if dialect_expr.dialect is OssieDialect.OSSIE_SQL_2026:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If ossie_expr.dialects has two OSSIE_SQL_2026 entries (I think it's legal from a schema perspective as Expression.dialects has minItems: 1 but no uniqueItems), ossie_sql_expr gets overwritten on each match and the one wins.

Maybe worth breaking on first OSSIE_SQL_2026 match, or documenting why last wins is intended.

I ack it's corner case, but wanted to flag it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed — the first OSSIE_SQL_2026 entry now wins, as it already does for the converter's own dialect.

I did not break on the match, though: the converter's dialect outranks OSSIE_SQL_2026 and can appear later in the list, so breaking makes [OSSIE_SQL_2026, ANSI_SQL] return the portable expression when ANSI_SQL is the target — it fails the existing test_ansi_sql_still_takes_precedence_over_ossie_sql_2026[ossie_sql_first]. Guarded the assignment with is None instead. Reasoning is in the docstring; test added for the repeated dialect.

Review follow-ups on apache#464.

_find_dataset_for_col still read field.expression.dialects[0] positionally,
so it and _get_expression could resolve the same field to different text and
a metric could be attributed to the wrong dataset. It now goes through
_get_expression, which makes it an instance method; its only call site
already went through self.

A dialect may also be listed more than once, since Expression.dialects has
minItems but no uniqueItems. The first OSSIE_SQL_2026 entry now wins, as the
converter's own dialect already does. The loop is not cut short on that match
because the converter's dialect outranks OSSIE_SQL_2026 and may appear later
in the list.
@jbonofre
jbonofre merged commit ac54fbf into apache:main Sep 25, 2026
4 checks passed
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.

2 participants