Repository navigation
feat(databricks): label imported expressions with their most portable dialect - #547
Conversation
… dialect Import (Metric View -> Apache Ossie) used to label every expression DATABRICKS, so a model taken Ossie -> Metric View -> Ossie came back Databricks-only even when it was plain ANSI or Ossie SQL. Each imported expression is now labeled OSSIE_SQL_2026 when it uses only the Apache Ossie expression language, else ANSI_SQL when it uses only standard SQL, else DATABRICKS. The new ExpressionDialect check is lexical and conservative: anything it does not recognize stays DATABRICKS. It rejects constructs Databricks reads differently (double-quoted strings, 'a''b', backslash escapes, TIMESTAMP, DATEDIFF, VARCHAR/FLOAT/integer CAST, windows) and names that are not a dataset column (the source. alias, struct fields, parameters, dimensions that may shadow a column). A measure with a window or partition, or any measure of a filtered view, stays DATABRICKS because those settings change its value and ride only in the DATABRICKS stash. Export already prefers DATABRICKS, then ANSI_SQL, then OSSIE_SQL_2026, so MV -> Ossie -> MV stays lossless. Co-authored-by: Isaac <no-reply@databricks.com>
…ATABRICKS Review follow-up to the import dialect labels. Each of these was labeled portable although Databricks resolves the name to something other than a column: - In a measure, a name outside an aggregate call refers to another measure (`cost / row_count`, or a window measure times 2), so it now stays DATABRICKS. - `dataset.name` now gets the same shadow check as a bare name: with a dimension `o_amount: o_amount * 100`, `SUM(source.o_amount)` reads the raw column, while the Apache Ossie `orders.o_amount` names the field. - A view with an unnamed dimension other than a fact wildcard (a joined `customer.*`, or a derived name) adds dimensions the import cannot see, so every expression in it stays DATABRICKS. Also rejects the `<<` / `>>` shifts and `localtime`, `current_time`, `grouping__id`, corrects the comments on `'a''b'` (one literal by default, two concatenated only under a legacy setting), and adds tests pinning each guard. Co-authored-by: Isaac <no-reply@databricks.com>
kayemkim
left a comment
There was a problem hiding this comment.
I round-tripped the hello world model from Chris's thread through this branch. SUM(amount) comes back OSSIE_SQL_2026 whether it went in as ANSI_SQL or OSSIE_SQL_2026, where main returned DATABRICKS for both. The README rules held on the cases I tried: a measure over a derived dimension (SUM(net) with net: amount - 1) and every measure under a view filter stay DATABRICKS, CHAR_LENGTH lands on ANSI_SQL, and TRUNC, CURRENT_TIME and CAST(x AS INT) stay DATABRICKS as documented.
One design question. dialects is a list, and many expressions here fit both OSSIE_SQL_2026 and ANSI_SQL. Emitting every label that fits, with the same text, instead of only the most portable one would let a model that went in as ANSI_SQL come back with that label intact, which is what the thread's fidelity check counts, and a consumer that only knows ANSI_SQL could still read it. Export order would not change. The cost is a duplicated expression text per field, so I can see the argument either way.
One note for later: SUM(amount) FILTER (WHERE amount > 0) is labeled ANSI_SQL, which is right against the spec as it stands. Once #382 lands, FILTER would belong in the Ossie set too.
jbonofre
left a comment
There was a problem hiding this comment.
LGTM, thanks for this!
The dialect classifier reads cleanly, and the demotion of view filter, window and partition to DATABRICKS is handled well. The new labels also still round-trip through the OssieConverterCommon fallback.
One small thing that could be fixed: || is in SYMBOLS and falls into the default -> true branch of symbol(), so it is never marked as non-Ossie. An expression like a || b can end up labeled OSSIE_SQL_2026, but core-spec/expression_language.md doesn't list || as an operator. A spec-following consumer could reject or misparse it.
Suggested fix: handle || in symbol() like % and !=. Clear ossie and keep ansi, so it gets the ANSI_SQL label. A test covering a a || b would be good too.
|
Thanks, @jbonofre. I noticed something interesting in the
|
That's a good find - that looks like a rendering issue (there's been a few of those we've had to fix). I'm happy to file a PR with you as co-author |
I'd initially bias towards most portable. IIRC, the ANSI dialect comes from early OSI before the expression language had even been fleshed out. For now, Ossie SQL is generally a subset of ANSI. The distinction will become more interesting if Ossie supports functions / constructs outside of ANSI. Then, you might need to export both. @kayemkim if you have the time, I'd be curious if you'd like to join in on the parent thread to help seed some of the initial measurement test cases / methodology. |




Summary
Addresses the dev@ thread on dialect round trips (https://lists.apache.org/thread/dnl13g1nlh7l2o6gnw9qjp7wqmrh0tds): importing a Metric View used to label every expression
DATABRICKS, so a model taken Apache Ossie -> Metric View -> Apache Ossie came back Databricks-only even when it was plain ANSI or Ossie SQL.Import now labels each expression with the most portable dialect it fits:
OSSIE_SQL_2026when it uses only the Apache Ossie expression language, elseANSI_SQLwhen it uses only standard SQL, elseDATABRICKS.Before and after, for the hello world model (one
salesdataset,SUM(sales.amount)) written inANSI_SQLorOSSIE_SQL_2026:The check (
ExpressionDialect) is lexical, not a parser, and deliberately conservative. Anything it does not recognize staysDATABRICKS, which is always correct for an expression read from a Metric View. A construct counts as portable only if it is allowlisted and, where the spec defines its behavior, Databricks evaluates it the same way. An expression staysDATABRICKSwhen:"x"(a string in Databricks),'a''b'(two concatenated literals under a legacy setting), backslash escapes,TIMESTAMP(session time zone),DATEDIFF(counts whole elapsed units), aVARCHAR,FLOAT, integer, or bareDECIMALCAST, a window (OVER, where NULLs sort first by default), or one-argumentLOG,COUNT(a, b),CEIL(x, scale), and the<</>>shifts.source.in a dimension, a parameter, or a dimension that may shadow a column (bare or asdataset.name). In a measure, any name outside an aggregate call counts as a reference to another measure (cost / row_count). A view with an unnamed dimension other than*orsource.*(such ascustomer.*) keeps everythingDATABRICKS, since its expanded names may shadow columns.windoworpartition, or any measure of a view with afilter. These settings change the value and live only in theDATABRICKSstash.Export already prefers
DATABRICKS, thenANSI_SQL, thenOSSIE_SQL_2026, so MV -> Apache Ossie -> MV stays lossless. Behavior the spec leaves open (integer division, NULL handling inCONCAT/GREATEST, rounding of ties, the result type ofDATE_TRUNC/DATEADD, date unit names) is listed in the README as not checked. This is a first, crude pass meant to cover the basic cases; it can be extended as the spec settles.Related Issues
Checklist
Specification
core-spec/and follow the existing structureOntology
ontology/are consistent with spec changesConverters
converters/is updated to reflect spec or ontology changes.github/labeler.ymlis updated with the new pathValidation
validation/are updated if the spec changedDocumentation
docs/is updated to reflect any user-facing changesCONTRIBUTING.mdis updated if the contribution process changedExamples
examples/are added or updated for any new spec constructs or converter supportTests
pytest/ CI green)Compliance
NOTICEand/orLICENSEhave been updated per ASF policyThis pull request and its description were written by Isaac.