Decompose semantic catalogs from runtime graphs - #329
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 602c499ed6
ℹ️ 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".
| def _runtime_state(runtime: SemanticGraph) -> object: | ||
| """Capture all semantic graph state while excluding derived caches.""" | ||
| return ( | ||
| runtime.models, |
There was a problem hiding this comment.
When a compiled graph comes from adapters that add semantic attributes directly to the graph, such as Omni's topics, Snowflake's verified_queries/instructions, or TMDL passthrough state, mutating those attributes after scope construction is not detected because this fixed tuple only captures the nine built-in fields. cache_key therefore continues returning the old compilation identity, and clone_runtime() can bind a layer containing the changed adapter state; include all non-cache graph state in the snapshot rather than enumerating only these attributes.
Useful? React with 👍 / 👎.
| layer.compiled_scope = compiled_scope | ||
| # Runtime layers receive isolated clones. Query-time or user mutation | ||
| # in one layer must not mutate the compiled catalog or another layer. | ||
| layer.graph = compiled_scope.clone_runtime() |
There was a problem hiding this comment.
If a scope's runtime is mutated after compilation, clone_runtime() raises CompiledSemanticScopeMutationError here after cls(...) has already opened the database adapter. Unlike the dialect-mismatch branch, this failure path never closes the layer, so repeated rejected bindings against PostgreSQL, Snowflake, or another remote adapter leak live connections; ensure every post-construction binding failure closes the adapter.
Useful? React with 👍 / 👎.
| logical_data_type: str | None = Field( | ||
| None, | ||
| exclude=True, | ||
| description="Source logical data type, independent of the effective runtime dimension type", | ||
| ) | ||
| declared_is_time: bool | None = Field( | ||
| None, | ||
| exclude=True, |
There was a problem hiding this comment.
For any model participating in extends, merge_model() converts both parent and child definitions through model_dump() before reconstructing their dimensions, and exclude=True unconditionally removes logical_data_type and declared_is_time. The same serialization path drops the new metric logical type and relationship edge_id, so inherited definitions silently lose the loss-aware IR metadata introduced by this change; the inheritance merge needs an explicit preservation path for these excluded fields.
Useful? React with 👍 / 👎.
1 participant
Stack created with GitHub Stacks CLI • Give Feedback 💬