Fix invalid AT (SET year = year - 1) binder case#19
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: 4d059e256b
ℹ️ 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".
| if c.is_alphabetic() || c == '_' { | ||
| let mut token = String::from(c); | ||
| while let Some(&next) = chars.peek() { | ||
| if next.is_alphanumeric() || next == '_' { | ||
| token.push(chars.next().unwrap()); |
There was a problem hiding this comment.
expr_mentions_identifier currently treats any alphabetic run as an identifier, even inside string literals, so a valid constant assignment like AT (SET region = 'region') with no GROUP BY region is now rejected as a self-reference. This introduces false validation errors for legitimate constant SET expressions on text dimensions; the identifier scan needs to skip quoted literal content.
Useful? React with 👍 / 👎.
yardstick-rs/src/sql/measures.rs
Outdated
| group_by_cols | ||
| .iter() | ||
| .any(|col| col.split('.').next_back().unwrap_or(col).eq_ignore_ascii_case(dim_name)) |
There was a problem hiding this comment.
dimension_in_group_by compares only the final segment after . (for example, both s.year and c.year become year), which can let invalid SET self-references pass validation in join queries. In cases where the measure expansion references one table alias but only another alias’s column is grouped, this check returns true and the rewritten SQL still hits the DuckDB binder error the new guard is intended to prevent.
Useful? React with 👍 / 👎.
1 participant
Adds validation for AT (SET dim = expr) when the expression references the same dimension but that dimension is not in query grouping context. Instead of emitting SQL that later fails with a DuckDB binder error, expansion now returns a direct Yardstick error explaining how to fix the query. Also adds regression coverage for all three cases: invalid self-reference without grouping, valid self-reference with grouping, and valid constant SET without grouping. Verified with cargo test in yardstick-rs (69 passed).