fix(wisdom): preserve ONE_TO_MANY relationship type on round-trip - #455
tripleaceme wants to merge 2 commits into
Conversation
The Wisdom to Ossie converter swaps from/to for ONE_TO_MANY edges but set no ai_context note, so the reverse converter fell back to MANY_TO_ONE. Emit a "one-to-many relationship" note like the existing one-to-one and many-to-many ones, and have the reverse converter recognise it and swap the sides and columns back. Fix the test expectation that encoded the wrong type and add a round-trip test checking every representable fixture relationship comes back with its type and direction. Closes apache#425
kayemkim
left a comment
kayemkim
left a comment
There was a problem hiding this comment.
Thanks for picking this up after asking on the issue first. Ran the branch merged into current main (2b33615) through the wisdom CI steps on 3.11 to 3.14: green, 29 tests against 27. Pushing sample_export.json through both converters by hand, main returns the second edge as MANY_TO_ONE orders -> customers, and this branch returns ONE_TO_MANY customers.customer_id -> orders.customer_id, matching the source, with the other three edges unchanged and no issues raised. Merges cleanly with #449. LGTM from me.
| if isinstance(relationship.ai_context, str): | ||
| if relationship.ai_context.startswith("one-to-one"): | ||
| relationship_type = "ONE_TO_ONE" | ||
| elif relationship.ai_context.startswith("one-to-many"): |
There was a problem hiding this comment.
startswith makes ai_context act as a direction marker. A hand-written Ossie relationship with something like "one-to-many, orders to customers" in ai_context will now be flipped on export, with no warning.
That's easy to hit because ai_context is a free-text field.
Could we match the exact string the forward path writes (== "one-to-many relationship"), or define the marker as a constant shared by both converters? The existing one-to-one and many-to-many checks have the same weakness, so this could be a follow-up if you'd rather keep the PR small.
There was a problem hiding this comment.
Good point. I fixed it in this PR for all three markers rather than leaving a follow-up. The notes are now defined once in RELATIONSHIP_TYPE_NOTES in wisdom_to_ossie.py. The forward path writes from it, and ossie_to_wisdom imports it and matches exactly. Any other note, including "one-to-many, orders to customers", now stays MANY_TO_ONE and is reported as AI_CONTEXT_DROPPED. I added a test for that case. (721780f)
| from_dataset, to_dataset = right, left | ||
| from_columns = [pair[1] for pair in column_pairs] | ||
| to_columns = [pair[0] for pair in column_pairs] | ||
| ai_context = "one-to-many relationship" |
There was a problem hiding this comment.
Does this override an ai_context that was already set on the relationship? If the wisdom source can carry its own note, it may be worth appending to it instead.
There was a problem hiding this comment.
It doesn't override anything. A Wisdom relationship only carries joinCondition and relationshipType in its properties, so there's no source note to keep. ai_context starts empty and is only ever set to the type marker.
| elif relationship.ai_context.startswith("one-to-many"): | ||
| # The forward path swapped sides to put the many side in `from`; swap back. | ||
| relationship_type = "ONE_TO_MANY" | ||
| left, right = right, left |
There was a problem hiding this comment.
The swap changes left and right, and left_ref and right_ref use them, so the ordering matters. A short comment saying those refs must be built after this point would stop someone moving the swap later.
There was a problem hiding this comment.
Added a comment at the swap saying left_ref, right_ref and the join conditions must be built after it. (721780f)
ossie_to_wisdom read any ai_context starting with "one-to-one", "one-to-many" or "many-to-many" as a type marker, so a hand-written note such as "one-to-many, orders to customers" flipped the relationship on export with no warning. The notes are now defined once in RELATIONSHIP_TYPE_NOTES and matched exactly; any other note is reported as AI_CONTEXT_DROPPED. Also note that the join refs must be built after the ONE_TO_MANY swap.
jbonofre
left a comment
jbonofre
left a comment
There was a problem hiding this comment.
Thanks!
One small thing, I believe the wisdom README.md still describes the old behavior.
In the "Relationship cardinality" table, the ONE_TO_MANY row has no ai_context note, and, under "Ossie -> wisdom", the relationships bullet says the notes restore only ONE_TO_ONE/MANY_TO_MANY.
Nothing says the note has to match exactly. That is worth stating, because editing the note now changes the exported cardinality.
For a follow-up, with exact matching, the notes are effectively a wire format stored in a free-text field. That has two side effects:
- a hand-written note such as "one-to-one" or "many-to-many relationship" no longer restores the type
ai_context: one-to-many relationshipsits on a relationship whosefromis the many side, so the text reads as contradicting the model
Moving the markers into WISDOM custom_extensions, as the cube, microsoft and databricks converters already do, would fix both. I will open an issue for it.
3 participants
Summary
A Wisdom
ONE_TO_MANYrelationship came back asMANY_TO_ONEafter Wisdom → Ossie → Wisdom. The forward path swapped from/to but set no marker, and the reverse path defaulted unmarked relationships toMANY_TO_ONE.wisdom_to_ossie.py: theONE_TO_MANYbranch now setsai_context = "one-to-many relationship", in the same format as the one-to-one and many-to-many markers.ossie_to_wisdom.py: a matchingone-to-manybranch restores the type and swaps the tables and column lists back, so the relationship comes out in its original direction.test_relationship_types_restoredexpected the wrong result; it now expectsONE_TO_MANYat index 1.New tests:
sample_export.jsonthat compares every relationship before and afterONE_TO_MANYcase with different column names on each side (customer_fk/id). The fixture usescustomer_idon both sides, so it can't catch a column swap on its own.On the 5 → 4 relationship count mentioned in the issue: that is intended. The fifth relationship in the fixture joins on an
ORcondition, which an Ossie relationship can't represent. It's dropped with aRELATIONSHIP_DROPPEDissue atwisdom_to_ossie.py:257-263, andtest_or_join_is_droppedalready covers it.@mwu-wisdom, since you were cc'd on the issue, happy to hand this over or adjust if you had a different approach in mind.
Related Issues
Closes #425
Tests
cd converters/wisdom && uv run pytest: 29 passed. The three new or updated tests fail without the fix.