Skip to content

Commit b99d546

Browse files
authored
fix(tesseract): reduce_by/grain matches time dimensions regardless of granularity (cube-js#11188)
1 parent e55263b commit b99d546

10 files changed

Lines changed: 286 additions & 4 deletions

rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/multi_stage/member_query_planner.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -542,15 +542,15 @@ impl MultiStageMemberQueryPlanner {
542542
let dimensions = if let Some(exclude) = &grain.exclude {
543543
dimensions
544544
.into_iter()
545-
.filter(|d| !exclude.iter().any(|m| d.has_member_in_reference_chain(m)))
545+
.filter(|d| !exclude.iter().any(|m| d.matches_grain_reference(m)))
546546
.collect_vec()
547547
} else {
548548
dimensions
549549
};
550550
let dimensions = if let Some(keep_only) = &grain.keep_only {
551551
dimensions
552552
.into_iter()
553-
.filter(|d| keep_only.iter().any(|m| d.has_member_in_reference_chain(m)))
553+
.filter(|d| keep_only.iter().any(|m| d.matches_grain_reference(m)))
554554
.collect_vec()
555555
} else {
556556
dimensions

rust/cube/cubesqlplanner/cubesqlplanner/src/planner/planners/multi_stage/multi_stage_query_planner.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -294,15 +294,15 @@ impl MultiStageQueryPlanner {
294294
) -> Vec<Rc<MemberSymbol>> {
295295
let dims: Vec<Rc<MemberSymbol>> = if let Some(exclude) = &grain.exclude {
296296
dims.iter()
297-
.filter(|d| !exclude.iter().any(|m| d.has_member_in_reference_chain(m)))
297+
.filter(|d| !exclude.iter().any(|m| d.matches_grain_reference(m)))
298298
.cloned()
299299
.collect()
300300
} else {
301301
dims.clone()
302302
};
303303
if let Some(keep_only) = &grain.keep_only {
304304
dims.into_iter()
305-
.filter(|d| keep_only.iter().any(|m| d.has_member_in_reference_chain(m)))
305+
.filter(|d| keep_only.iter().any(|m| d.matches_grain_reference(m)))
306306
.collect()
307307
} else {
308308
dims

rust/cube/cubesqlplanner/cubesqlplanner/src/planner/symbols/member_symbol.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,23 @@ impl MemberSymbol {
236236
false
237237
}
238238

239+
/// Whether this projected member is targeted by a grain reference
240+
/// (`reduce_by` / `group_by`). Behaves like `has_member_in_reference_chain`,
241+
/// but a time dimension also matches through its underlying base dimension:
242+
/// a projected time dimension carries a granularity suffix in its full name
243+
/// (`created_at_month`) while grain references point at the bare dimension
244+
/// (`created_at`), so a grain reference removes the time dimension from the
245+
/// grain regardless of the granularity it is queried at.
246+
pub fn matches_grain_reference(&self, member: &Rc<MemberSymbol>) -> bool {
247+
if self.has_member_in_reference_chain(member) {
248+
return true;
249+
}
250+
if let Self::TimeDimension(td) = self {
251+
return td.base_symbol().has_member_in_reference_chain(member);
252+
}
253+
false
254+
}
255+
239256
/// Returns a copy of this symbol with the path reduced to just the owning cube,
240257
/// stripping any join chain prefix (e.g. from views or cross-cube references).
241258
pub fn with_stripped_join_prefix(&self) -> Rc<Self> {

rust/cube/cubesqlplanner/cubesqlplanner/src/test_fixtures/schemas/yaml_files/common/integration_multi_stage.yaml

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,17 @@ cubes:
186186
reduce_by:
187187
- orders.status
188188

189+
# reduce_by targets the time dimension itself (bare, no
190+
# granularity). When the query selects created_at at some
191+
# granularity, that granular dimension must be dropped from the
192+
# partition regardless of its granularity suffix (CORE-550).
193+
- name: amount_reduce_time
194+
type: sum
195+
sql: "{CUBE.total_amount}"
196+
multi_stage: true
197+
reduce_by:
198+
- orders.created_at
199+
189200
- name: amount_reduce_all
190201
type: sum
191202
sql: "{CUBE.total_amount}"
@@ -290,6 +301,20 @@ cubes:
290301
- sql: "{CUBE.total_amount}"
291302
dir: desc
292303

304+
# Rank reduced by the time dimension (bare) while ordered by a
305+
# measure. With created_at selected at a granularity the partition
306+
# must be "all outer dims except created_at" (CORE-550); the buggy
307+
# planner left created_at_<gran> in PARTITION BY so every partition
308+
# held one row and all ranks collapsed to 1.
309+
- name: amount_rank_reduce_time
310+
type: rank
311+
multi_stage: true
312+
reduce_by:
313+
- orders.created_at
314+
order_by:
315+
- sql: "{CUBE.total_amount}"
316+
dir: desc
317+
293318
# Rank by the raw time dimension itself (order_by is a
294319
# dimension, not a measure) reduced by created_at — partition
295320
# is "all outer dims except created_at". First link in the
@@ -533,3 +558,5 @@ views:
533558
- total_amount
534559
- amount_prev_month
535560
- mom_growth
561+
- amount_reduce_time
562+
- amount_rank_reduce_time

rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/rank.rs

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,66 @@ fn create_context() -> TestContext {
99

1010
const SEED: &str = "integration_multi_stage_tables.sql";
1111

12+
/// PARTITION BY clause bodies (text between `PARTITION BY` and the following
13+
/// `ORDER BY`) of every window function in `sql`.
14+
fn partition_by_clauses(sql: &str) -> Vec<String> {
15+
sql.match_indices("PARTITION BY")
16+
.map(|(i, _)| {
17+
let rest = &sql[i + "PARTITION BY".len()..];
18+
let end = rest.find("ORDER BY").unwrap_or(rest.len());
19+
rest[..end].to_string()
20+
})
21+
.collect()
22+
}
23+
24+
// CORE-550: a rank reduced by a bare time dimension, ordered by a measure,
25+
// queried with that time dimension at a granularity. Partition must be
26+
// [status] alone; the buggy planner kept created_at_month in PARTITION BY so
27+
// each (status, month) partition held one row and every rank was 1. Correct
28+
// ranks per status by descending monthly total (completed Mar>Feb>Jan = 1,2,3;
29+
// pending 1,2,3; cancelled Mar=1, Jan/Feb tie at 2).
30+
#[tokio::test(flavor = "multi_thread")]
31+
async fn test_rank_reduce_by_time_dimension() {
32+
let ctx = create_context();
33+
34+
let query = indoc! {r#"
35+
measures:
36+
- orders.total_amount
37+
- orders.amount_rank_reduce_time
38+
dimensions:
39+
- orders.status
40+
time_dimensions:
41+
- dimension: orders.created_at
42+
granularity: month
43+
dateRange:
44+
- "2024-01-01"
45+
- "2024-03-31"
46+
order:
47+
- id: orders.status
48+
- id: orders.created_at
49+
"#};
50+
51+
let sql = ctx.build_sql(query).unwrap();
52+
for clause in partition_by_clauses(&sql) {
53+
assert!(
54+
!clause.contains("created_at"),
55+
"reduce_by(created_at) must drop the time dimension from PARTITION BY,\n\
56+
partition clause was: {}\nfull SQL:\n{}",
57+
clause,
58+
sql,
59+
);
60+
assert!(
61+
clause.contains("status"),
62+
"status must remain in PARTITION BY, partition clause was: {}",
63+
clause,
64+
);
65+
}
66+
67+
if let Some(result) = ctx.try_execute_pg(query, SEED).await {
68+
insta::assert_snapshot!(result);
69+
}
70+
}
71+
1272
#[tokio::test(flavor = "multi_thread")]
1373
async fn test_basic_rank() {
1474
let ctx = create_context();

rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/reduce_by.rs

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,18 @@ fn assert_no_window(sql: &str) {
2727
);
2828
}
2929

30+
/// PARTITION BY clause bodies (text between `PARTITION BY` and the following
31+
/// `ORDER BY`) of every window function in `sql`.
32+
fn partition_by_clauses(sql: &str) -> Vec<String> {
33+
sql.match_indices("PARTITION BY")
34+
.map(|(i, _)| {
35+
let rest = &sql[i + "PARTITION BY".len()..];
36+
let end = rest.find("ORDER BY").unwrap_or(rest.len());
37+
rest[..end].to_string()
38+
})
39+
.collect()
40+
}
41+
3042
// add_group_by + reduce_by together: leaf grain extends with customer_id
3143
// while partition grain shrinks by removing category. Three distinct grains:
3244
// leaf = (status, category, customer_id) ← per-customer sum(amount)
@@ -317,6 +329,57 @@ async fn test_reduce_by_count_distinct() {
317329
}
318330
}
319331

332+
// CORE-550: reduce_by a bare time dimension must drop it from the window's
333+
// PARTITION BY even when the query selects that dimension at a granularity
334+
// (auto-suffixed `_month`). The buggy planner matched reduce_by references by
335+
// full_name, so `orders.created_at` never matched the granular
336+
// `orders.created_at_month`; created_at stayed in the partition, the window
337+
// collapsed to plain group-by and the "reduced" value equalled total_amount.
338+
// Correct: partition = [status] → per-status total broadcast across months
339+
// (completed=1400, pending=650, cancelled=200).
340+
#[tokio::test(flavor = "multi_thread")]
341+
async fn test_reduce_by_time_dimension() {
342+
let ctx = create_context();
343+
344+
let query = indoc! {r#"
345+
measures:
346+
- orders.total_amount
347+
- orders.amount_reduce_time
348+
dimensions:
349+
- orders.status
350+
time_dimensions:
351+
- dimension: orders.created_at
352+
granularity: month
353+
dateRange:
354+
- "2024-01-01"
355+
- "2024-03-31"
356+
order:
357+
- id: orders.status
358+
- id: orders.created_at
359+
"#};
360+
361+
let sql = ctx.build_sql(query).unwrap();
362+
assert_uses_window(&sql);
363+
for clause in partition_by_clauses(&sql) {
364+
assert!(
365+
!clause.contains("created_at"),
366+
"reduce_by(created_at) must drop the time dimension from PARTITION BY,\n\
367+
partition clause was: {}\nfull SQL:\n{}",
368+
clause,
369+
sql,
370+
);
371+
assert!(
372+
clause.contains("status"),
373+
"status must remain in PARTITION BY, partition clause was: {}",
374+
clause,
375+
);
376+
}
377+
378+
if let Some(result) = ctx.try_execute_pg(query, SEED).await {
379+
insta::assert_snapshot!(result);
380+
}
381+
}
382+
320383
#[tokio::test(flavor = "multi_thread")]
321384
async fn test_reduce_by_with_time() {
322385
let ctx = create_context();
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
source: cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/rank.rs
3+
expression: result
4+
---
5+
orders__status | orders__created_at_month | orders__total_amount | orders__amount_rank_reduce_time
6+
---------------+--------------------------+----------------------+--------------------------------
7+
cancelled | 2024-01-01 00:00:00 | 50.00 | 2
8+
cancelled | 2024-02-01 00:00:00 | 50.00 | 2
9+
cancelled | 2024-03-01 00:00:00 | 100.00 | 1
10+
completed | 2024-01-01 00:00:00 | 300.00 | 3
11+
completed | 2024-02-01 00:00:00 | 500.00 | 2
12+
completed | 2024-03-01 00:00:00 | 600.00 | 1
13+
pending | 2024-01-01 00:00:00 | 150.00 | 3
14+
pending | 2024-02-01 00:00:00 | 200.00 | 2
15+
pending | 2024-03-01 00:00:00 | 300.00 | 1
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
source: cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/reduce_by.rs
3+
expression: result
4+
---
5+
orders__status | orders__created_at_month | orders__total_amount | orders__amount_reduce_time
6+
---------------+--------------------------+----------------------+---------------------------
7+
cancelled | 2024-01-01 00:00:00 | 50.00 | 200.00
8+
cancelled | 2024-02-01 00:00:00 | 50.00 | 200.00
9+
cancelled | 2024-03-01 00:00:00 | 100.00 | 200.00
10+
completed | 2024-01-01 00:00:00 | 300.00 | 1400.00
11+
completed | 2024-02-01 00:00:00 | 500.00 | 1400.00
12+
completed | 2024-03-01 00:00:00 | 600.00 | 1400.00
13+
pending | 2024-01-01 00:00:00 | 150.00 | 650.00
14+
pending | 2024-02-01 00:00:00 | 200.00 | 650.00
15+
pending | 2024-03-01 00:00:00 | 300.00 | 650.00
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
source: cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/views.rs
3+
expression: result
4+
---
5+
orders_ms_view__status | orders_ms_view__created_at_month | orders_ms_view__total_amount | orders_ms_view__amount_reduce_time
6+
-----------------------+----------------------------------+------------------------------+-----------------------------------
7+
cancelled | 2024-01-01 00:00:00 | 50.00 | 200.00
8+
cancelled | 2024-02-01 00:00:00 | 50.00 | 200.00
9+
cancelled | 2024-03-01 00:00:00 | 100.00 | 200.00
10+
completed | 2024-01-01 00:00:00 | 300.00 | 1400.00
11+
completed | 2024-02-01 00:00:00 | 500.00 | 1400.00
12+
completed | 2024-03-01 00:00:00 | 600.00 | 1400.00
13+
pending | 2024-01-01 00:00:00 | 150.00 | 650.00
14+
pending | 2024-02-01 00:00:00 | 200.00 | 650.00
15+
pending | 2024-03-01 00:00:00 | 300.00 | 650.00

rust/cube/cubesqlplanner/cubesqlplanner/src/tests/integration/multi_stage/views.rs

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,76 @@ fn create_context() -> TestContext {
99

1010
const SEED: &str = "integration_multi_stage_tables.sql";
1111

12+
fn assert_uses_window(sql: &str) {
13+
assert!(
14+
sql.contains("OVER (PARTITION BY"),
15+
"expected SQL to use a window function (`... OVER (PARTITION BY ...)`),\n\
16+
got:\n{}",
17+
sql,
18+
);
19+
}
20+
21+
/// PARTITION BY clause bodies (text between `PARTITION BY` and the following
22+
/// `ORDER BY`) of every window function in `sql`.
23+
fn partition_by_clauses(sql: &str) -> Vec<String> {
24+
sql.match_indices("PARTITION BY")
25+
.map(|(i, _)| {
26+
let rest = &sql[i + "PARTITION BY".len()..];
27+
let end = rest.find("ORDER BY").unwrap_or(rest.len());
28+
rest[..end].to_string()
29+
})
30+
.collect()
31+
}
32+
33+
// CORE-550 through a view: the query groups by view dimensions
34+
// (orders_ms_view.status / .created_at) while the measure's reduce_by targets
35+
// the underlying base cube member (orders.created_at). The granular view time
36+
// dimension (created_at_month) must still be dropped from the window's
37+
// PARTITION BY, leaving [status]. Guards that the fix resolves the reduce_by
38+
// reference through the view→base reference chain, not just for plain cubes.
39+
#[tokio::test(flavor = "multi_thread")]
40+
async fn test_view_reduce_by_time_dimension() {
41+
let ctx = create_context();
42+
43+
let query = indoc! {r#"
44+
measures:
45+
- orders_ms_view.total_amount
46+
- orders_ms_view.amount_reduce_time
47+
dimensions:
48+
- orders_ms_view.status
49+
time_dimensions:
50+
- dimension: orders_ms_view.created_at
51+
granularity: month
52+
dateRange:
53+
- "2024-01-01"
54+
- "2024-03-31"
55+
order:
56+
- id: orders_ms_view.status
57+
- id: orders_ms_view.created_at
58+
"#};
59+
60+
let sql = ctx.build_sql(query).unwrap();
61+
assert_uses_window(&sql);
62+
for clause in partition_by_clauses(&sql) {
63+
assert!(
64+
!clause.contains("created_at"),
65+
"reduce_by(created_at) must drop the view time dimension from PARTITION BY,\n\
66+
partition clause was: {}\nfull SQL:\n{}",
67+
clause,
68+
sql,
69+
);
70+
assert!(
71+
clause.contains("status"),
72+
"status must remain in PARTITION BY, partition clause was: {}",
73+
clause,
74+
);
75+
}
76+
77+
if let Some(result) = ctx.try_execute_pg(query, SEED).await {
78+
insta::assert_snapshot!(result);
79+
}
80+
}
81+
1282
#[tokio::test(flavor = "multi_thread")]
1383
async fn test_view_with_time_shift() {
1484
let ctx = create_context();

0 commit comments

Comments
 (0)