Skip to content

Commit 4ab83db

Browse files
committed
Handle GROUP BY spacing
1 parent 0f1f4ca commit 4ab83db

File tree

2 files changed

+37
-3
lines changed

2 files changed

+37
-3
lines changed

test/sql/measures.test

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,20 @@ SELECT * FROM a;
5050
2023 EU 75.0
5151
2023 US 150.0
5252

53+
# =============================================================================
54+
# Test: GROUP BY with extra spaces
55+
# =============================================================================
56+
57+
query IIR rowsort
58+
SEMANTIC SELECT year, region, AGGREGATE(revenue) AT (ALL region) AS year_total
59+
FROM sales_v
60+
GROUP BY year, region;
61+
----
62+
2022 EU 150.0
63+
2022 US 150.0
64+
2023 EU 225.0
65+
2023 US 225.0
66+
5367
# =============================================================================
5468
# Test: AT (ALL dimension) - remove dimension from context
5569
# =============================================================================

yardstick-rs/src/sql/measures.rs

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -839,6 +839,26 @@ fn find_first_top_level_keyword(sql: &str, start: usize, keywords: &[&str]) -> O
839839
.min()
840840
}
841841

842+
fn has_top_level_group_by(sql: &str) -> bool {
843+
find_top_level_keyword(sql, "GROUP BY", 0).is_some()
844+
}
845+
846+
fn has_group_by_anywhere(sql: &str) -> bool {
847+
let upper = sql.to_uppercase();
848+
let mut idx = 0;
849+
while idx < upper.len() {
850+
if matches_keyword_at(&upper, idx, "GROUP") {
851+
let mut next = idx + "GROUP".len();
852+
next = skip_whitespace(sql, next);
853+
if matches_keyword_at(&upper, next, "BY") {
854+
return true;
855+
}
856+
}
857+
idx += 1;
858+
}
859+
false
860+
}
861+
842862
fn skip_whitespace(sql: &str, mut idx: usize) -> usize {
843863
while idx < sql.len() && sql.as_bytes()[idx].is_ascii_whitespace() {
844864
idx += 1;
@@ -1996,7 +2016,7 @@ fn extract_measures_from_sql(
19962016
.iter()
19972017
.any(|m| find_aggregation_in_expression(&m.expression).is_some());
19982018
let clean_sql_upper = clean_sql.to_uppercase();
1999-
let has_group_by = clean_sql_upper.contains("GROUP BY");
2019+
let has_group_by = has_top_level_group_by(&clean_sql);
20002020

20012021
if has_aggregate_measure && !has_group_by {
20022022
// Find insertion point: before ORDER BY, LIMIT, or at end
@@ -2181,7 +2201,7 @@ pub fn expand_aggregate(sql: &str) -> AggregateExpandResult {
21812201

21822202
// Check if we need to add GROUP BY
21832203
let result_upper = result_sql.to_uppercase();
2184-
if !result_upper.contains("GROUP BY") {
2204+
if !has_group_by_anywhere(&result_sql) {
21852205
// Extract dimension columns (non-aggregate items)
21862206
let dim_cols = extract_dimension_columns_from_select_info(&select_info);
21872207
if !dim_cols.is_empty() {
@@ -3796,7 +3816,7 @@ pub fn expand_aggregate_with_at(sql: &str) -> AggregateExpandResult {
37963816
// If no GROUP BY, add explicit GROUP BY with dimension columns from original SQL
37973817
// (GROUP BY ALL doesn't work reliably with scalar subqueries mixed with aggregates)
37983818
let result_upper = result_sql.to_uppercase();
3799-
if !result_upper.contains("GROUP BY") && !original_dim_cols.is_empty() {
3819+
if !has_group_by_anywhere(&result_sql) && !original_dim_cols.is_empty() {
38003820
// Find insertion point: before ORDER BY, LIMIT, HAVING, or at end
38013821
let insert_pos = ["ORDER BY", "LIMIT", "HAVING", ";"]
38023822
.iter()

0 commit comments

Comments
 (0)