[OSSIE][DATABRICKS] Add Java Converter and Reformat - #333
Conversation
366cccf to
5572106
Compare
|
@Haoranli503 I pushed some fixes here. Can you please take a look? |
|
Hi @jbonofre , fixes look good. Thanks! I also pushed one commit addressing some issues we found when we are building native sql support based on the converter. Hope you can review those as well. Also feel free to change. |
|
Thanks @Haoranli503! |
Move the Python converter under converters/databricks/python/ (content unchanged) and add a Maven Java module -- library, CLI (OssieDatabricksConverter), JUnit tests, and fixtures under java/, package org.apache.ossie.converter.databricks -- as the maintained implementation. Add a root README describing the two-language layout, and a Java build job (mvn -B verify, JDK 21) in converter-databricks-ci.yml, mirroring the polaris converter. Signed-off-by: Haoran Li <haoran.li@databricks.com>
Replace the Python README's forward-looking 'Future effort' section with a deprecation note: the Java converter under java/ is the maintained implementation; the Python copy is kept for reference and no longer actively extended. Signed-off-by: Haoran Li <haoran.li@databricks.com>
Build: - maven-shade no longer writes dependency-reduced-pom.xml into the module root, where apache-rat failed `mvn verify` on it as an unapproved file - configure surefire to include **/*Suite.java: the default includes match none of the test classes, so the build ran zero tests and still passed - drop the **/*.md rat exclude and restore the ASF header on both READMEs - align snakeyaml with the 2.3 that jackson-dataformat-yaml declares Converter: - qualifyMeasure matches the whole qualifier run and resolves it from the leaf, so an expression that already carries a join path is no longer qualified a second time (SUM(customer.customer.region.population)) - de-alias measure qualifiers on import, the inverse of the export rewrite and what resolveColumn already did for dimensions - match dropped names outside string literals when cascading drops, so a name that only occurs in a literal no longer drops an unrelated column - quoteReplacement the stash unicode-escape pass, which halved an escaped backslash run instead of re-emitting it verbatim - notice the ai_context object members and the foreign-vendor extensions dropped from a field or a metric - validate a join source on import with the rule the export applies, so a view that imports cleanly is always exportable again CLI: - name the directions from the Apache Ossie model's point of view, matching the library Javadoc and the Python CLI: export = Ossie -> Metric View - give each command its own selector flag instead of sharing one field, and resolve the command before parsing arguments so --help prints usage - print to stdout without the extra newline, so stdout and -o agree Tests: - generate join-qualified and nested-path measures, and one_to_many branches, in the property round-trip suites - add a regression test per fix, plus a CLI suite (the CLI had none)
Signed-off-by: Haoran Li <haoran.li@databricks.com>
…und trip A dimension's display_name maps to the Ossie Field label, but the Ossie Metric schema has no label, so a metric view measure's display_name was dropped in the MV -> Ossie -> MV round trip. Preserve it in the DATABRICKS custom_extensions stash (the same mechanism as format/window) in MetricViewToOssie.convertMeasure, and restore it in OssieToMetricView.convertMetric. Ports apache#326 (landed in Databricks runtime as databricks-eng/runtime#251390). Co-authored-by: Isaac <no-reply@databricks.com>
… complex_joins stash A Metric View join whose `on` is not an equi-join of simple `alias.column` pairs (a non-equi operator, a SQL-function-wrapped key, or an extra filter predicate) has no Apache Ossie relationship form: the relationship schema requires from_columns/to_columns. The converter used to abort the whole MV -> Ossie conversion on such a join. Instead of aborting, preserve the join under the model's DATABRICKS custom_extensions (complex_joins) and warn, rather than emitting a schema-invalid stub relationship with no columns. The reverse converter merges the stashed joins back into the relationship graph and restores each raw `on` verbatim, so such a metric view round-trips (nesting and one_to_many included). Condition-less (cross) joins still have no Apache Ossie representation and are still rejected. Also preserve an equi-join's original `on` verbatim when rebuilding it from the from/to columns would not reproduce it (a fact side qualified by the source table name rather than `source`, or an `on` over equal columns that would rebuild as `using`); canonical joins stash nothing. Ports apache#321 (landed in Databricks runtime as databricks-eng/runtime#251398). Co-authored-by: Isaac <no-reply@databricks.com>
9b270b8 to
7d94fc6
Compare
…g tables Upstream apache#356 clarified the mapping table (a measure's display_name has no `label` on the Apache Ossie metric shape, so it rides in the DATABRICKS stash). The converter restructure turned the top-level README into a short pointer, so carry that clarification into the mapping tables now in python/README.md and java/README.md. Both converters stash a measure's display_name (Python via apache#356, Java via the measure display_name round-trip commit), so the tables match the behavior. Co-authored-by: Isaac <no-reply@databricks.com>
|
Hi @jbonofre , all fixes are in. Can you help tal? Thanks! |
| if (!isFact) { | ||
| // A dataset reachable by more than one path (diamond) appears once per path; the first | ||
| // wins, matching the order dimensions are emitted in. | ||
| datasetAliasPath.putIfAbsent(node.dataset, qualifier); |
There was a problem hiding this comment.
A dataset reached via two distinct join paths (a diamond) is silently mapped to only its first-seen alias path via putIfAbsent. A measure expression qualified by this dataset's bare name (SUM(shared.amount)) will always resolve to one arbitrary branch with no warning (unlike the dimension-path handling of the same ambiguity, which explicitily warns and drops).
For instance, fact orders -> dim1 -> shared and orders -> dim2 -> shared, plus SUM(shared.amount), silently emits SUM(dim1.dim1_shared.amount) and drops the dim2 branch.
Can we warn-and-drop here the same way the dimension path does?
There was a problem hiding this comment.
Good catch. Fixed: a measure that names a diamond dataset by its bare name is now dropped with a notice, the same way the dimension path drops a complex expression it cannot unambiguously qualify. A measure addresses a dataset by name, so it cannot be split per branch the way a dimension column is, which makes warn and drop the consistent choice. Added a test measureNamingADiamondDatasetIsDroppedWithNotice covering the orders to dim1/dim2 to shared example.
| return model; | ||
| } | ||
|
|
||
| private static void walk(String parentName, String parentAlias, List<Object> joins, |
There was a problem hiding this comment.
The import direction (walk/hasOtm) never enforces MAX_JOIN_NODES, while export checks it before and during recursion. A Metric View with 250+ joins imports cleanly but then fails to re-export.
I believe it breaks the documented guarantee that anything that imports cleanly can be exported again.
Should we add the check on import too?
There was a problem hiding this comment.
Good catch, added the bound. MetricViewToOssie (MV to Ossie) now rejects a Metric View whose fact plus joins exceed MAX_JOIN_NODES, the same limit OssieToMetricView already enforces, so the two directions stay symmetric and the round trip holds. One note on the framing: the failure is actually the reverse. A Metric View with 200 or more joins exports cleanly today, but the emitted model then fails to import again, since the MAX_JOIN_NODES checks live on the Ossie to MV side. The fix rejects it at export instead. Added tests exportRejectsMoreJoinsThanImportCanRebuild and exportAtTheJoinLimitStillSucceeds.
| } | ||
| } | ||
|
|
||
| private static void markOtmVisit(String modelName, Node node, boolean branchIsOtm) { |
There was a problem hiding this comment.
markOtmVisit rejects a join whose cardinality differs from its top-level branch's cardinality in either direction.
The Python reference (_mark_otm) only rejects reverting from one-to-many back to many-to-one: it explicitly allows a one-to-many join nested under a many-to-one branch. As written, a normal star-schema-with-fanout model (many-to-one to customer, then one-to-many to customer_notes) that Python accepts gets rejected here with "is one_to_many but descends from a many_to_one join".
Is the stricter check intentional, or should this match the Python behavior?
There was a problem hiding this comment.
This one is intentional. The Java matches the Databricks engine's actual rule. A Metric View validates each top level join branch with Join.validateSubJoinCardinalities, seeded from that branch's top level join cardinality, and fails any descendant whose cardinality differs in either direction (Join.scala, invoked per top level join by SingleSourceMetricView.validateJoins). So your example, many_to_one to customer then one_to_many to customer_notes, is actually rejected by the engine at CREATE: Expected 'customer_notes' to have 'many_to_one' cardinality, but found 'one_to_many'. The Python _mark_otm is the one out of step: it only rejects reverting from one_to_many back to many_to_one, so it would emit that view and hit the engine error at CREATE instead. Its leniency does not actually help, it just moves the same rejection from conversion time to CREATE time. The Python copy is deprecated so we left it as is.
| Map<String, Object> stash = null; | ||
| for (Map<String, Object> ext : customExtensions(obj)) { | ||
| if (VENDOR.equals(str(get(ext, "vendor_name")))) { | ||
| if (stash != null) { |
There was a problem hiding this comment.
readStash throws when more than one DATABRICKS custom_extensions entry is present.
Python's read_stash just returns the first match and ignores later duplicates.
An Ossie YAML with two such entries that Python converts fine gets rejected here.
Worth aligning with the Python behavior (first-wins) rather than erroring?
There was a problem hiding this comment.
Good point, changed it. readStash no longer throws on a duplicate DATABRICKS custom_extensions entry. It keeps the first, ignores the rest to match first wins, and emits a converter notice so the dropped entry is not lost silently rather than vanishing the way a plain first wins would. So a model with two entries converts instead of failing, and the user still sees that one was ignored. Added a test duplicateDatabricksStashKeepsFirstAndWarns.
| // cannot represent it (from/to columns are required). Rather than abort the whole | ||
| // conversion, warn here; the columns-less entry built below is stashed under the model's | ||
| // custom_extensions (complex_joins) so it round-trips, instead of an invalid relationship. | ||
| notices.warn("join '" + child + "'", "non-equi or unsupported join condition ('on: " + rawOn |
There was a problem hiding this comment.
This accepts a non-equi/filtered join on clause and preserves it via the new complex_joins stash, but the Python reference still raises ConversionError on the same input.
Is this divergence intentional/tracked, or should Java match Python's current (stricter) behavior?
There was a problem hiding this comment.
Intentional and tracked. A join condition Apache Ossie cannot represent as a relationship (a non equi comparison, a function wrapped key, or an extra filter predicate) has no schema valid relationship form, since an Ossie relationship requires from_columns and to_columns. Rather than abort the whole export, the Java preserves the raw clause in a model level complex_joins stash inside custom_extensions, and the reverse converter rebuilds it. That keeps the round trip lossless for a join that carries real business logic, and the emitted model stays schema valid with no misleading columns less stub relationship, so a non Databricks consumer simply does not see the stashed join rather than seeing a wrong one. The Python reference still raises because it is the deprecated copy.
…th a notice A measure addresses a dataset by name, so a bare reference to a dataset reached by more than one join path (a diamond) cannot be unambiguously qualified. Warn and drop it, mirroring how the dimension path handles a complex expression on a diamond, instead of silently binding to one arbitrary branch.
…round trip symmetric The reverse conversion rejects a model with more than MAX_JOIN_NODES datasets, but the forward conversion had no matching bound, so a Metric View with too many joins could export to a model that could never be imported again. Reject it at export instead, with the same limit.
…stead of failing A duplicate DATABRICKS custom_extensions entry is malformed input. Rather than reject the whole conversion, readStash now keeps the first entry, ignores the rest, and emits a notice so the dropped entry is not lost silently.
|
Hi @jbonofre Thanks for the careful pass, these are good catches. Some context that ties several together: the Java converter is now the maintained, on track implementation. It tracks the current Metric View engine rules, and it carries the newer decisions (for example the So where Java and Python differ, Java is the intended source of truth:
The other three are improvements we applied:
Please help re-verify, thanks! |
…avior Document the join-count bound on both conversion directions, the diamond-measure and duplicate-stash notices, and the model-level complex_joins stash for a non-representable join 'on' (dropping the stale line that listed non-equi joins as rejected).
5b2da58 to
2de9015
Compare
Databricks SQL identifiers are case-insensitive, but cascade-drop matched references to dropped fields and metrics case-sensitively, so a metric such as COUNT(DISTINCT REGION_NAME) survived referencing a dropped region_name as a dangling reference. Make both the pre-filter and the regex case-insensitive in the propagation gate (matches) and the confirmation (referencesDropped), and compile referencePattern with CASE_INSENSITIVE. Adds a test with the reported repro. Fixes apache#422. Co-authored-by: Isaac <no-reply@databricks.com>
Brings the branch current with main (including apache#396, which migrated the other converters and the Python SDK to flat, model-at-root semantic documents). Resolves the converters/databricks README conflict by keeping this branch's overview, since the Java and Python implementations live in their own subdirectories here. Migrating the Java converter itself to flat documents is a follow-up (it needs a build environment to verify). Co-authored-by: Isaac <no-reply@databricks.com>
… documents Match apache/ossie apache#396: an Ossie document carries the model directly at the document root (version, name, datasets, relationships, metrics) rather than under a semantic_model wrapper. Import reads the root model and rejects a legacy semantic_model wrapper, a missing string name, and root dialects/vendors; export emits the model at the root. Migrates all test inputs and fixtures to the flat format and re-roots the export assertions. Verified by compiling the converter and running a driver (flat import, legacy rejection, flat export, round trip) and converting all fixtures. The JUnit assertions still need a mvn run as the final gate. Co-authored-by: Isaac <no-reply@databricks.com>
|
Pushed three commits. a991e68 (fixes #422): cascade drop matched references to dropped fields case sensitively, so a metric like COUNT(DISTINCT REGION_NAME) survived referencing a dropped region_name as a dangling reference. It now matches case insensitively, in both the containment prefilter and the regex, across the propagation gate and the confirmation. Added a test with the reported repro. 53c59f6: merged main into the branch to bring it current with #396. Resolved the one converters/databricks/README.md conflict by keeping this branch's overview, since the Java and Python implementations live in their own subdirectories here. 416119f: migrated the Java converter to flat documents (the model at the document root) to match #396. Import reads the model at the root and rejects a legacy semantic_model wrapper, a missing string name, and root dialects/vendors; export emits the model at the root. Flattened all test inputs and fixtures and updated the export assertions to read the model at the root. One note on verification for that last commit: I could not run mvn locally, so I compiled the converter and ran a driver covering flat import, the legacy rejection, flat export, and a round trip, and reconverted all three fixtures. The JUnit assertions still need a mvn test run as the final gate. |
| + "[{dialect: DATABRICKS, expression: 'measure(m1)'}]}}\n" | ||
| + " - {name: bad_dim, expression: {dialects: " | ||
| + "[{dialect: SNOWFLAKE, expression: bad_dim}]}}\n" | ||
| + " metrics:\n" |
There was a problem hiding this comment.
This YAML literal indents metrics: at 2 spaces, which nests it under the dataset item instead of putting it at the model root.
schemaMapList(model, "metrics", ...) returns empty, so m1/m0/bad_measure/keep never get parsed.
Running mvn test on this PR fails as a result (1 cascade notice observed vs 4 expected).
metrics: needs re-indented to column 0.
There was a problem hiding this comment.
Fixed. The metrics: block was nested under the dataset by mistake; moved it to the model root so the cascade test sees all four notices again.
| List<String> parentCols = (List<String>) decomposed[0]; | ||
| List<String> childCols = (List<String>) decomposed[1]; | ||
| String rawOn = (String) decomposed[2]; | ||
| if (rawOn != null) { |
There was a problem hiding this comment.
This unconditionally warns-and-stashes a non-equi/unsupported join condition rather than rejecting it.
I see two issues:
- it diverges from the Python reference, which raises
ConversionErrorfor the same input (test_non_equi_on_rejected) - I believe issue [databricks] MV→Ossie import aborts on non-equi/filtered join conditions instead of preserving them in custom_extensions #321, cited as justification, asked for this to be opt-in, not unconditional.
Should this be gated behind a flag, or should it reject like Python does?
There was a problem hiding this comment.
Confirmed offline with Chris, went with rejecting, like Python. Fixed!
| writeStash(rel, stash); | ||
| relationships.add(rel); | ||
| } | ||
| String factHint = explicitSource != null ? explicitSource : str(get(modelStash, STASH_SOURCE_KEY)); |
There was a problem hiding this comment.
explicitSource is checked with a plain null-check, so an explicit empty string (e.g. --source "$SRC" with $SRC unset) is treated as a real override instead of "absent".
That throws ConversionException("requested source '' is not a dataset") instead of falling back to the model source_dataset hint.
Python uses a truthy check here: this should match (explicitSource != null && !explicitSource.isEmpty() or similar).
There was a problem hiding this comment.
Good catch, fixed. explicitSource now uses a truthy check (explicitSource != null && !explicitSource.isEmpty()), matching Python's explicit_source or ..., so an empty --source falls back to the model hint instead of failing as requested source '' is not a dataset. Added a test.
| boolean isSql = SELECT_WITH_RE.matcher(source.toString().trim()).find(); | ||
| String lastId = lastIdentifier(source); | ||
| String factName; | ||
| if (modelName != null) { |
There was a problem hiding this comment.
Same pattern as OssieToMetricView: modelName is checked with if (modelName != null) instead of a truthy check, so --name "" produces name: "" in the output instead of falling back to deriving the name from the source table.
Python falls back for the same input: this should too I guess.
…e-order test The flat-document migration left `metrics:` indented under the dataset item in cascadeDropPreservesDimensionThenMeasurePhaseOrder, so `schemaMapList(model, "metrics", ...)` returned empty and the metrics (m1/m0/bad_measure/keep) never parsed. The test then saw 1 cascade notice instead of 4. Re-indent `metrics:` and its items to column 0 (the model root). Verified against the compiled converter: it now emits the 4 expected cascade notices in order. Co-authored-by: Isaac <no-reply@databricks.com>
…check) An empty --source (for example an unset shell variable) was non-null, so it was taken as a real override and failed as "requested source '' is not a dataset" instead of falling back to the model source hint. Match the Python converter's truthy check (explicit_source or ...): only a non-empty explicit source overrides. Added a test that an empty source falls back exactly like an absent one. Co-authored-by: Isaac <no-reply@databricks.com>
…uthy check) An empty --name (for example an unset shell variable) was non-null, so MetricViewToOssie took it as a literal model name and emitted name: "" instead of falling back to deriving the name from the source's last identifier. Match the Python converter's truthy check (model_name or ...): only a non-empty name overrides. Added a test that an empty name falls back exactly like an absent one. Co-authored-by: Isaac <no-reply@databricks.com>
Port apache#446 to the Java converter. pickExpression only tried DATABRICKS then ANSI_SQL, so a field or metric written solely in OSSIE_SQL_2026 (Apache Ossie's portable, ANSI-SQL-compatible dialect, added to the spec in apache#439 and apache#440) fell through to null and was dropped from the Metric View. Add OSSIE_SQL_2026 to the fallback chain (DATABRICKS, then ANSI_SQL, then OSSIE_SQL_2026), update the two drop warnings, and add four regression tests (field-only, metric-only, DATABRICKS-preferred precedence, unsupported-dialect still dropped). The import direction only ever writes DATABRICKS, so it is untouched. Co-authored-by: Isaac <no-reply@databricks.com>
…port Match the Python converter (test_non_equi_on_rejected / test_complex_equi_on_rejected): a join `on` that is not an equi-join of simple `alias.column` pairs has no Apache Ossie relationship form (from/to columns are required), so reject it on import rather than stashing it under the model's DATABRICKS custom_extensions. Issue apache#321 asked for the preserve behavior to be opt-in with the default unchanged; the Java port had made it the unconditional default, diverging from Python. Remove the Java-only complex_joins machinery on both the import (produce) and export (rebuild) sides -- Python has no such concept -- and rework the round-trip tests into rejection tests. Decomposable equi-joins, including a fact side qualified by the source table name, are unaffected and still round-trip their `on` verbatim. Co-authored-by: Isaac <no-reply@databricks.com>
Brings in apache#446 (OSSIE_SQL_2026 on the Python converter, already ported to Java on this branch) plus other recent main work. Clean merge: git maps main's converters/databricks/src edits onto this branch's moved converters/databricks/python copy, so the Python converter stays in sync. Co-authored-by: Isaac <no-reply@databricks.com>
jbonofre
left a comment
jbonofre
left a comment
There was a problem hiding this comment.
LGTM, thanks a lot!
I think there is some possible improvements (especially comparing with the Python converter), but let's have this converter landed and enhance as follow-ups.
3 participants
Summary
Adds a Java implementation of the Databricks converter alongside the existing Python
reference, and restructures
converters/databricks/to host both languages. The Java library isthe implementation maintained going forward; the Python converter becomes a reference copy slated
for deprecation.
Conversion is unchanged in spirit: pure YAML text in, YAML text out, reading and writing the two
formats as parsed maps and lists. The Java and Python implementations target the same Databricks
Unity Catalog Metric View YAML v1.1 contract, and the checked-in fixtures pin the expected output
for both directions (
export: Ossie -> Metric View,import: Metric View -> Ossie).Changes:
src/,tests/,pyproject.toml,README.md,uv.lock) moves underconverters/databricks/python/with no content changes,making room for the Java tree.
java/— the bidirectional converter as a Maven module: library, anOssieDatabricksConvertercommand-line tool, JUnit 5 tests, and YAML fixtures. Packageorg.apache.ossie.converter.databricks, following the conventions of the existingpolarisandsalesforceJava converters (parentorg.apache:apache:39; deps SnakeYAML / Jackson / JUnit 5;apache-rat-pluginlicense audit; shade plugin for the executable CLI jar).README.mddescribing the two-language layout and pointing at eachimplementation's own README.
.github/workflows/converter-databricks-ci.yml) — point the Python job at the newpython/working directory, and add a Java job (mvn -B verify, Temurin JDK 21) mirroringconverter-polaris-ci.yml.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 changesValidation
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