Skip to content

Harden Malloy compatibility boundaries - #334

Merged
nicosuave merged 7 commits into
mainfrom
malloy-compatibility
Aug 24, 2026
Merged

Harden Malloy compatibility boundaries#334
nicosuave merged 7 commits into
mainfrom
malloy-compatibility

Conversation

@nicosuave

Copy link
Copy Markdown
Member

Makes Malloy imports fail closed instead of silently changing unsupported semantics. Adds invariant source filters, intrinsic schema exposure, exact and role-aliased joins, module visibility, typed dialect-aware expressions, bounded one-stage query mapping, structured fidelity diagnostics, and transactional loader behavior.\n\nAdds focused Python coverage and an opt-in official Malloy differential harness, and rewrites the compatibility documentation around the actual supported boundary.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 620bbacde0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +20 to +25
target_model: str | None = Field(
default=None,
description=(
"Canonical related model when the relationship name is a role alias. "
"When omitted, name remains the target model for backwards compatibility."
),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Teach exporters to resolve the canonical role target

When an imported Malloy join uses an alias such as buyer is customers, name is now buyer while the graph only contains the canonical customers model. Existing exporters still resolve targets through relationship.name; for example, Cube uses resolved_models.get(relationship.name) in sidemantic/adapters/cube.py:1795, and TMDL uses graph.models.get(rel.name) in sidemantic/adapters/tmdl.py:1834, so CLI conversions silently omit these joins. Update downstream exporters to use related_model while preserving the role where supported, or explicitly block the conversion where aliases cannot be represented.

AGENTS.md reference: AGENTS.md:L7-L9

Useful? React with 👍 / 👎.

Comment on lines +2836 to +2840
Relationship(
name=name,
target_model=target_name if aliased_reference else None,
type=rel_type,
foreign_key=self._collapse_join_keys(foreign_keys),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Exclude aliased relationships from FK inference

For directory/CLI loading, _infer_relationships runs after Malloy parsing but checks whether a relationship already targets a model using r.name == target (sidemantic/loaders.py:1613-1618). With join_one: buyer is customers on customer_id = buyer.id, this relationship has name='buyer' and target_model='customers', so a customer_id dimension causes inference to add a second canonical customers relationship and a reverse edge. This exposes a join path absent from the Malloy source and can introduce ambiguity; inference should compare r.related_model for role-aliased relationships.

AGENTS.md reference: AGENTS.md:L7-L8

Useful? React with 👍 / 👎.

@nicosuave
nicosuave merged commit 04a3dcf into main Aug 24, 2026
25 of 26 checks passed
@nicosuave
nicosuave deleted the malloy-compatibility branch August 24, 2026 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant