Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions converters/sigma/src/ossie_sigma/converter_issues.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class ConverterIssueType(Enum):
UNSUPPORTED_ELEMENT_KIND = "UNSUPPORTED_ELEMENT_KIND"
EXPRESSION_NOT_TRANSLATABLE = "EXPRESSION_NOT_TRANSLATABLE"
RELATIONSHIP_COLUMN_UNRESOLVED = "RELATIONSHIP_COLUMN_UNRESOLVED"
RELATIONSHIP_COLUMN_ARITY_MISMATCH = "RELATIONSHIP_COLUMN_ARITY_MISMATCH"
UNIQUE_KEY_COLUMN_UNRESOLVED = "UNIQUE_KEY_COLUMN_UNRESOLVED"
DERIVED_ELEMENT_NOT_MODELED = "DERIVED_ELEMENT_NOT_MODELED"
FILTER_NOT_MODELED = "FILTER_NOT_MODELED"
Expand Down
15 changes: 14 additions & 1 deletion converters/sigma/src/ossie_sigma/ossie_to_sigma.py
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ def _build_element(
relationships = relationships_by_element.get(element_id, [])
if relationships:
element["relationships"] = [
self._build_relationship(r, dataset.name, dataset_element_id, field_ids) for r in relationships
self._build_relationship(r, dataset.name, dataset_element_id, field_ids, issues) for r in relationships
]

return element
Expand Down Expand Up @@ -346,6 +346,7 @@ def _build_relationship(
dataset_name: str,
dataset_element_id: dict[str, str],
field_ids: dict[str, str],
issues: list[ConverterIssue],
) -> dict[str, Any]:
ext = _sigma_ext(rel) or {}
target_element_id = dataset_element_id.get(rel.to, rel.to)
Expand All @@ -365,6 +366,18 @@ def _build_relationship(
if raw_keys is not None:
result["keys"] = raw_keys
else:
if len(rel.from_columns) != len(rel.to_columns):

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.

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.

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.

@khush-bhatia FYI, see my request change here (about ConverterError).

# zip() below stops at the shorter array; record what it drops.
issues.append(
ConverterIssue(
ConverterIssueType.RELATIONSHIP_COLUMN_ARITY_MISMATCH,
f"{dataset_name}.{rel.name}",
f"from_columns ({len(rel.from_columns)}) and to_columns "
f"({len(rel.to_columns)}) have different lengths; the "
f"{abs(len(rel.from_columns) - len(rel.to_columns))} extra "
"key column(s) were dropped from the Sigma relationship.",
)
)
result["keys"] = [
{
"sourceColumnId": field_ids.get(from_col, from_col),
Expand Down
84 changes: 84 additions & 0 deletions converters/sigma/tests/test_ossie_to_sigma.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,90 @@ def test_empty_semantic_model_raises_a_clear_error():
OssieToSigmaConverter().convert(document)


def test_relationship_column_arity_mismatch_is_recorded_not_silently_truncated():
"""from_columns/to_columns are independently constrained in the OSI schema (each
only needs to be non-empty), so a compound-key relationship with unequal lengths
is legal input. zip() truncates to the shorter array; that must be a recorded
issue, not a silent drop of the extra key column(s)."""
document = OssieDocument(
semantic_model=[
OssieSemanticModel(
name="m",
datasets=[
OssieDataset(name="orders", source="db.public.orders"),
OssieDataset(name="regions", source="db.public.regions"),
],
relationships=[
OssieRelationship(
name="OrderRegion",
**{"from": "orders"},
to="regions",
from_columns=["region_id", "sub_id"],
to_columns=["region_id"],
),
],
)
]
)

result = OssieToSigmaConverter().convert(document)

issue_types = {i.issue_type for i in result.issues}
assert ConverterIssueType.RELATIONSHIP_COLUMN_ARITY_MISMATCH in issue_types

rel = next(
r for p in result.output["pages"] for e in p["elements"] for r in e.get("relationships", [])
)
# The mismatch is still recorded rather than crashing the conversion, but only
# one key pair can be formed from a 2-vs-1 mismatch.
assert len(rel["keys"]) == 1


def test_relationship_arity_mismatch_element_names_are_scoped_by_owning_dataset():
"""Relationship identity is already scoped by (dataset_name, rel.name) (see
test_relationship_ids_are_scoped_by_owning_dataset); an arity-mismatch issue's
element_name must be scoped the same way, or two unrelated relationships sharing a
name on different table pairs become indistinguishable in the issue list."""
document = OssieDocument(
semantic_model=[
OssieSemanticModel(
name="m",
datasets=[
OssieDataset(name="orders", source="db.public.orders"),
OssieDataset(name="customers", source="db.public.customers"),
OssieDataset(name="shipments", source="db.public.shipments"),
OssieDataset(name="carriers", source="db.public.carriers"),
],
relationships=[
OssieRelationship(
name="Parent",
**{"from": "orders"},
to="customers",
from_columns=["region_id", "sub_id"],
to_columns=["region_id"],
),
OssieRelationship(
name="Parent",
**{"from": "shipments"},
to="carriers",
from_columns=["region_id", "sub_id"],
to_columns=["region_id"],
),
],
)
]
)

result = OssieToSigmaConverter().convert(document)

arity_issues = [
i for i in result.issues if i.issue_type == ConverterIssueType.RELATIONSHIP_COLUMN_ARITY_MISMATCH
]
assert len(arity_issues) == 2
element_names = {i.element_name for i in arity_issues}
assert len(element_names) == 2, "arity-mismatch issues for same-named relationships must not collide"


def test_model_level_metadata_round_trips_through_ossie_and_back():
spec = load_fixture("fixtureA_sigma.json")
spec.update(
Expand Down