Conversation
…ave unequal length from_columns and to_columns are only constrained independently in the OSI JSON schema (each just needs at least one entry), so a compound-key relationship with unequal-length arrays is legal input. _build_relationship paired them with a bare zip(), which stops at the shorter array and drops the extra key column(s) with no warning at all. Thread the issues list into _build_relationship and record a RELATIONSHIP_COLUMN_ARITY_MISMATCH issue when the lengths differ, mirroring the RELATIONSHIP_COLUMN_UNRESOLVED pattern already used for the reverse direction. The converter still degrades to the shorter pairing (Sigma's own keys array has no way to represent an unequal-length join), but the loss is now visible in the returned ConverterResult.issues instead of silent.
Relationship identity is already scoped by (dataset_name, rel.name); the arity-mismatch ConverterIssue used the bare relationship name, so two same-named relationships on different table pairs became indistinguishable when both hit the mismatch.
|
Good catch, fixed: the arity-mismatch issue now uses f"{dataset_name}.{rel.name}" for element_name, matching the relationship id's own scoping. Added test_relationship_arity_mismatch_element_names_are_scoped_by_owning_dataset (two same-named relationships on different table pairs, both arity-mismatched); against the prior code it fails with 1 == 2, both issues collapsing onto "Parent". Full 93-test suite passes in the repo's own verify command. Pushed 633ca26. |
| if raw_keys is not None: | ||
| result["keys"] = raw_keys | ||
| else: | ||
| if len(rel.from_columns) != len(rel.to_columns): |
There was a problem hiding this comment.
Now core-spec/spec.md (Relationship) states: "Both arrays must have the same number of columns". It's not something we should warn and proceed, it's a "real" issue.
The PR here handles the identical condition by recording a ConverterIssue and then converting via zip(), which silently truncates to the shorter array. That's inconsistent with the spec statement, and it means a document that skips validate.py gets corrupted output from the converter instead of being rejected.
We already have what we need: converter_issues.py defines ConverterError, raised when the input cannot converted at all, as opposed to partial, lossy conversion that a ConverterIssue an describe. This file already uses it for structurally invalid input (empty semantic_model).
An arity mismatch is a spec-invalid input, not a legal-but-lossy translation gap like EXPRESSION_NOT_TRANSLATABLE.
I suggest raising ConverterError here instead of appending a ConverterIssue and continuing.
There was a problem hiding this comment.
@khush-bhatia FYI, see my request change here (about ConverterError).
|
I'd keep the converter's own handling rather than switch to a hard fail: #375's arity check lives in validation/validate.py, a separate script the docs recommend running before conversion but nothing wires into ossie-sigma's CLI or model_validate() call. A document can reach OssieToSigmaConverter without ever going through that script, so this path is still reachable with real input, not just a defensive-programming guard against something the schema now forbids. |
Summary
_build_relationshippairedfrom_columns/to_columnswith a barezip(), which stops at the shorter array. The OSI schema only requires each array to be non-empty independently, so a compound-key relationship with unequal lengths is legal input, and the extra key column(s) were dropped from the exported Sigma spec with no record of it.Thread
issuesinto_build_relationshipand record aRELATIONSHIP_COLUMN_ARITY_MISMATCHissue when the lengths differ, mirroring theRELATIONSHIP_COLUMN_UNRESOLVEDpattern already used for the reverse (Sigma-to-Ossie) direction. The conversion still degrades to the shorter pairing, since Sigma'skeysarray has no way to represent an unequal-length join, but the loss is now visible inConverterResult.issuesinstead of silent.Related Issues
Fixes #405
Checklist
Converters
converters/sigmais updatedTests
uv run pyteston Python 3.11, 3.12, 3.14 - 92 passed on eachmain: it errors there (the issue type doesn't exist yet)Compliance