Add Yardstick adapter and SQL rewrite parity#104
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: e1e9fa6877
ℹ️ 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".
sidemantic/sql/query_rewriter.py
Outdated
| for column in select_scope.find_all(exp.Column) | ||
| if not column.table and column.name in placeholder_names |
There was a problem hiding this comment.
_rewrite_yardstick_select_scope collects initial_placeholders with select_scope.find_all(exp.Column), which traverses nested subqueries/CTEs, so an outer query like SEMANTIC SELECT * FROM (SELECT AGGREGATE(revenue) FROM sales_v) s sees inner placeholders but has no semantic model in its own FROM and then raises Yardstick query must reference at least one known semantic model in FROM/JOIN. This breaks valid Yardstick queries that use subqueries, even though subquery support exists in the non-Yardstick rewrite path.
Useful? React with 👍 / 👎.
sidemantic/sql/generator.py
Outdated
| dim = model.get_dimension(col_name) | ||
| if dim: | ||
| dim_sql = replace_model_placeholder(dim.sql_expr) | ||
| select_cols.append(f"{dim_sql} AS {col_name}") |
There was a problem hiding this comment.
When injecting extra columns for inline-aggregate metric SQL, aliases are emitted as AS {col_name} without quoting, unlike the previous implementation. If a referenced source column is a reserved word or needs quoting (for example, order), generated CTE SQL becomes invalid or resolves to the wrong identifier, causing otherwise valid metrics with inline aggregates to fail at execution time.
Useful? React with 👍 / 👎.
1 participant
This adds Yardstick as a first-class adapter and expands SQL rewriting to cover nuanced Yardstick measure semantics, including AGGREGATE and AT forms. It updates validation, loader, dependency, generator, and aggregation detection paths so Yardstick models resolve through the same semantic pipeline. It ports the Yardstick replay fixture set and adds targeted rewriter regressions, including strict=false passthrough and default-alias scoping fixes. Local checks passed with ruff check, ruff format --check, and pytest -v (3131 passed).