Make semantic planning deterministic and faster - #304
Conversation
Apply rollup fallback across serving surfaces
Correct fanout and advanced metric planning
![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: a9139c32fb
ℹ️ 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".
| for model_b in model_list[i + 1 :]: | ||
| try: | ||
| graph.find_relationship_path(model_a, model_b) | ||
| graph.find_relationship_path(model_a, model_b, query_models=query_model_set) |
There was a problem hiding this comment.
When a query references a model that disambiguates a diamond—for example routes a -> b -> d and a -> c -> d with b explicitly selected—validation accepts the a -> b -> d route using query_models. The SQL generator's calls to find_relationship_path, including _add_join_paths_to_query, do not pass that context, so the same query raises AmbiguousJoinPathError during compilation when a becomes the base model (for example, because an a dimension is listed first). Carry the selected context into generation, or do not allow validation to accept a route that generation cannot reproduce.
Useful? React with 👍 / 👎.
| if next_model == to_model: | ||
| return new_path | ||
|
|
||
| queue.append((next_model, new_path)) | ||
|
|
||
| raise ValueError(f"No join path found between {from_model} and {to_model}") | ||
| shortest_length = len(new_path) | ||
| candidates[path_signature(new_path)] = new_path | ||
| elif shortest_length is None or len(new_path) < shortest_length: | ||
| queue.append((next_model, new_path, visited | {next_model})) |
There was a problem hiding this comment.
On a layered or otherwise highly connected semantic graph, this queue stores one state per complete simple route and candidates retains every equally short route. The number of shortest routes can be exponential in the number of models, so a validation or compile on a modest diamond-chain graph can consume excessive time and memory, whereas the previous BFS was linear. Track distances/predecessors or stop after retaining enough best candidates to establish ambiguity instead of enqueueing every route.
Useful? React with 👍 / 👎.
| safe = _acyclic_safe_set(graph) | ||
| if measure.name in safe: | ||
| return None |
There was a problem hiding this comment.
Because Metric instances are mutable and assigning fields such as metric.sql does not increment SemanticGraph._version, this graph-versioned safe set can become stale. After an acyclic graph is validated once, changing an existing derived metric to introduce a cycle causes subsequent validation to return here and miss the new cycle. Either invalidate the memo on metric mutation, include dependency definitions in the cache key, or avoid retaining acyclic results across validation runs.
Useful? React with 👍 / 👎.
1 participant
Extracts deterministic join-path selection, relationship inference correctness, pruned project loading, safe YAML loading, and linear validation from #279.