Auto preagg widgets and inline SQL metrics#60
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d3fbdf256
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if measure.agg is None and measure.sql and _sql_has_aggregation(measure.sql): | ||
| continue |
There was a problem hiding this comment.
Guard inline-agg SQL that uses unqualified columns
Allowing measure.agg is None when _sql_has_aggregation() is true means metrics like sql: "SUM(price) / SUM(cost)" now pass validation, but SQLGenerator._extract_metric_filter_columns only adds columns to the model CTE when they are table-qualified (it checks col.table), so unqualified price/cost references are skipped and the generated query will hit missing-column errors when it runs. This only happens for inline-aggregate SQL that doesn’t use {model}./qualified column names; consider enforcing qualification here or broadening column extraction to include unqualified column references for these metrics.
Useful? React with 👍 / 👎.
cf77a33 to
d505ae0
Compare
Summary:
Tests: