Fix Measures-in-SQL parity behavior and tests#22
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: 06834c4d05
ℹ️ 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".
yardstick-rs/src/sql/measures.rs
Outdated
| return info | ||
| .group_by_cols | ||
| .into_iter() | ||
| .map(|c| c.trim().to_string()) | ||
| .filter(|c| !c.is_empty() && !c.chars().all(|ch| ch.is_ascii_digit())) |
There was a problem hiding this comment.
This fast path now returns parser group_by_cols directly after filtering numeric entries, so queries like GROUP BY 1/GROUP BY 1,2 produce an empty list and never reach the fallback that infers grouping dimensions from the SELECT list. In expand_aggregate_with_at, that empty grouping context makes measure rewrites behave as if the query were ungrouped, which can collapse per-dimension results into global totals.
Useful? React with 👍 / 👎.
| case StatementType::SELECT_STATEMENT: { | ||
| auto sql_to_check = context.GetCurrentQuery(); |
There was a problem hiding this comment.
Because the EXTENSION_STATEMENT case has no terminating break, control now falls through into the new SELECT_STATEMENT rewrite path whenever parse_function != yardstick_parse. That means non-yardstick extension statements are unexpectedly passed through yardstick_has_aggregate/yardstick_expand_aggregate, and statements containing measure-like SQL text can be rebound as SELECT * FROM yardstick(...) instead of being handled by their own extension binder.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation