Skip to content

Commit fa2a89b

Browse files
paveltiunovpaveltiunov
authored
fix(cubesql): Replace only simple ungrouped measures in projections to avoid aggregate over aggregate statements (#7852)
1 parent 13ca394 commit fa2a89b

27 files changed

+342
-18
lines changed

‎rust/cubesql/cubesql/src/compile/mod.rs‎

Lines changed: 67 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4703,6 +4703,59 @@ from
47034703
);
47044704
}
47054705

4706+
// #[tokio::test]
4707+
// async fn push_down_measure_filter() {
4708+
// if !Rewriter::sql_push_down_enabled() {
4709+
// return;
4710+
// }
4711+
// init_logger();
4712+
//
4713+
// let query_plan = convert_select_to_query_plan(
4714+
// r#"SELECT
4715+
// SUM("KibanaSampleDataEcommerce"."sumPrice") AS "TEMP(Calculation_2760495522911424520)(2472686499)(0)",
4716+
// SUM(
4717+
// (
4718+
// CASE
4719+
// WHEN (
4720+
// "KibanaSampleDataEcommerce"."order_date" < (TIMESTAMP '2024-01-01 00:00:00.000')
4721+
// ) THEN "KibanaSampleDataEcommerce"."sumPrice"
4722+
// ELSE NULL
4723+
// END
4724+
// )
4725+
// ) AS "TEMP(Calculation_2760495522922868746)(243454951)(0)"
4726+
// FROM
4727+
// "public"."KibanaSampleDataEcommerce" "KibanaSampleDataEcommerce"
4728+
// HAVING
4729+
// (COUNT(1) > 0)
4730+
// "#
4731+
// .to_string(),
4732+
// DatabaseProtocol::PostgreSQL,
4733+
// )
4734+
// .await;
4735+
//
4736+
// let physical_plan = query_plan.as_physical_plan().await.unwrap();
4737+
// println!(
4738+
// "Physical plan: {}",
4739+
// displayable(physical_plan.as_ref()).indent()
4740+
// );
4741+
//
4742+
// let logical_plan = query_plan.as_logical_plan();
4743+
// assert_eq!(
4744+
// logical_plan.find_cube_scan().request,
4745+
// V1LoadRequestQuery {
4746+
// measures: Some(vec![]),
4747+
// dimensions: Some(vec![]),
4748+
// segments: Some(vec![]),
4749+
// time_dimensions: None,
4750+
// order: None,
4751+
// limit: None,
4752+
// offset: None,
4753+
// filters: None,
4754+
// ungrouped: Some(true),
4755+
// }
4756+
// );
4757+
// }
4758+
47064759
#[tokio::test]
47074760
async fn powerbi_inner_decimal_cast() {
47084761
init_logger();
@@ -15510,7 +15563,7 @@ limit
1551015563
}
1551115564
init_logger();
1551215565

15513-
let logical_plan = convert_select_to_query_plan(
15566+
let query_plan = convert_select_to_query_plan(
1551415567
r#"
1551515568
select
1551615569
"_"."t1.agentCountApprox" as "agentCountApprox",
@@ -15546,13 +15599,22 @@ limit
1554615599
.to_string(),
1554715600
DatabaseProtocol::PostgreSQL,
1554815601
)
15549-
.await
15550-
.as_logical_plan();
15602+
.await;
15603+
15604+
let physical_plan = query_plan.as_physical_plan().await.unwrap();
15605+
println!(
15606+
"Physical plan: {}",
15607+
displayable(physical_plan.as_ref()).indent()
15608+
);
1555115609

1555215610
assert_eq!(
15553-
logical_plan.find_cube_scan().request,
15611+
query_plan.as_logical_plan().find_cube_scan().request,
1555415612
V1LoadRequestQuery {
15555-
measures: Some(vec!["Logs.agentCountApprox".to_string(),]),
15613+
measures: Some(vec![
15614+
"Logs.agentCount".to_string(),
15615+
"Logs.agentCountApprox".to_string(),
15616+
"KibanaSampleDataEcommerce.count".to_string()
15617+
]),
1555615618
dimensions: Some(vec![
1555715619
"KibanaSampleDataEcommerce.taxful_total_price".to_string(),
1555815620
]),

‎rust/cubesql/cubesql/src/compile/rewrite/mod.rs‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -434,12 +434,14 @@ crate::plan_to_language! {
434434
member: Arc<LogicalPlan>,
435435
alias_to_cube: Vec<(String, String)>,
436436
ungrouped: bool,
437+
in_projection: bool,
437438
cube_members: Vec<LogicalPlan>,
438439
},
439440
WrapperPullupReplacer {
440441
member: Arc<LogicalPlan>,
441442
alias_to_cube: Vec<(String, String)>,
442443
ungrouped: bool,
444+
in_projection: bool,
443445
cube_members: Vec<LogicalPlan>,
444446
},
445447
// NOTE: converting this to a list might provide rewrite improvements
@@ -1263,23 +1265,25 @@ fn wrapper_pushdown_replacer(
12631265
members: impl Display,
12641266
alias_to_cube: impl Display,
12651267
ungrouped: impl Display,
1268+
in_projection: impl Display,
12661269
cube_members: impl Display,
12671270
) -> String {
12681271
format!(
1269-
"(WrapperPushdownReplacer {} {} {} {})",
1270-
members, alias_to_cube, ungrouped, cube_members
1272+
"(WrapperPushdownReplacer {} {} {} {} {})",
1273+
members, alias_to_cube, ungrouped, in_projection, cube_members
12711274
)
12721275
}
12731276

12741277
fn wrapper_pullup_replacer(
12751278
members: impl Display,
12761279
alias_to_cube: impl Display,
12771280
ungrouped: impl Display,
1281+
in_projection: impl Display,
12781282
cube_members: impl Display,
12791283
) -> String {
12801284
format!(
1281-
"(WrapperPullupReplacer {} {} {} {})",
1282-
members, alias_to_cube, ungrouped, cube_members
1285+
"(WrapperPullupReplacer {} {} {} {} {})",
1286+
members, alias_to_cube, ungrouped, in_projection, cube_members
12831287
)
12841288
}
12851289

‎rust/cubesql/cubesql/src/compile/rewrite/rules/wrapper/aggregate.rs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ impl WrapperRules {
3131
"?cube_scan_input",
3232
"?alias_to_cube",
3333
"?ungrouped",
34+
"?in_projection",
3435
"?cube_members",
3536
),
3637
"CubeScanWrapperFinalized:false",
@@ -46,37 +47,43 @@ impl WrapperRules {
4647
wrapped_select_projection_expr_empty_tail(),
4748
"?alias_to_cube",
4849
"?ungrouped",
50+
"WrapperPullupReplacerInProjection:false",
4951
"?cube_members",
5052
),
5153
wrapper_pushdown_replacer(
5254
"?group_expr",
5355
"?alias_to_cube",
5456
"?ungrouped",
57+
"WrapperPullupReplacerInProjection:false",
5558
"?cube_members",
5659
),
5760
wrapper_pushdown_replacer(
5861
"?aggr_expr",
5962
"?alias_to_cube",
6063
"?ungrouped",
64+
"WrapperPullupReplacerInProjection:false",
6165
"?cube_members",
6266
),
6367
wrapper_pullup_replacer(
6468
wrapped_select_window_expr_empty_tail(),
6569
"?alias_to_cube",
6670
"?ungrouped",
71+
"WrapperPullupReplacerInProjection:false",
6772
"?cube_members",
6873
),
6974
wrapper_pullup_replacer(
7075
"?cube_scan_input",
7176
"?alias_to_cube",
7277
"?ungrouped",
78+
"WrapperPullupReplacerInProjection:false",
7379
"?cube_members",
7480
),
7581
wrapped_select_joins_empty_tail(),
7682
wrapper_pullup_replacer(
7783
wrapped_select_filter_expr_empty_tail(),
7884
"?alias_to_cube",
7985
"?ungrouped",
86+
"WrapperPullupReplacerInProjection:false",
8087
"?cube_members",
8188
),
8289
wrapped_select_having_expr_empty_tail(),
@@ -86,6 +93,7 @@ impl WrapperRules {
8693
wrapped_select_order_expr_empty_tail(),
8794
"?alias_to_cube",
8895
"?ungrouped",
96+
"WrapperPullupReplacerInProjection:false",
8997
"?cube_members",
9098
),
9199
"WrappedSelectAlias:None",
@@ -112,13 +120,15 @@ impl WrapperRules {
112120
"?aggr_expr",
113121
"?alias_to_cube",
114122
"WrapperPullupReplacerUngrouped:true",
123+
"?in_projection",
115124
"?cube_members",
116125
),
117126
vec![("?aggr_expr", aggr_expr)],
118127
wrapper_pullup_replacer(
119128
"?measure",
120129
"?alias_to_cube",
121130
"WrapperPullupReplacerUngrouped:true",
131+
"?in_projection",
122132
"?cube_members",
123133
),
124134
self.pushdown_measure(

‎rust/cubesql/cubesql/src/compile/rewrite/rules/wrapper/aggregate_function.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ impl WrapperRules {
2222
agg_fun_expr("?fun", vec!["?expr"], "?distinct"),
2323
"?alias_to_cube",
2424
"?ungrouped",
25+
"?in_projection",
2526
"?cube_members",
2627
),
2728
agg_fun_expr(
@@ -30,6 +31,7 @@ impl WrapperRules {
3031
"?expr",
3132
"?alias_to_cube",
3233
"?ungrouped",
34+
"?in_projection",
3335
"?cube_members",
3436
)],
3537
"?distinct",
@@ -43,6 +45,7 @@ impl WrapperRules {
4345
"?expr",
4446
"?alias_to_cube",
4547
"?ungrouped",
48+
"?in_projection",
4649
"?cube_members",
4750
)],
4851
"?distinct",
@@ -51,6 +54,7 @@ impl WrapperRules {
5154
agg_fun_expr("?fun", vec!["?expr"], "?distinct"),
5255
"?alias_to_cube",
5356
"?ungrouped",
57+
"?in_projection",
5458
"?cube_members",
5559
),
5660
self.transform_agg_fun_expr("?fun", "?distinct", "?alias_to_cube"),

‎rust/cubesql/cubesql/src/compile/rewrite/rules/wrapper/alias.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,15 @@ impl WrapperRules {
1313
alias_expr("?expr", "?alias"),
1414
"?alias_to_cube",
1515
"?ungrouped",
16+
"?in_projection",
1617
"?cube_members",
1718
),
1819
alias_expr(
1920
wrapper_pushdown_replacer(
2021
"?expr",
2122
"?alias_to_cube",
2223
"?ungrouped",
24+
"?in_projection",
2325
"?cube_members",
2426
),
2527
"?alias",
@@ -32,6 +34,7 @@ impl WrapperRules {
3234
"?expr",
3335
"?alias_to_cube",
3436
"?ungrouped",
37+
"?in_projection",
3538
"?cube_members",
3639
),
3740
"?alias",
@@ -40,6 +43,7 @@ impl WrapperRules {
4043
alias_expr("?expr", "?alias"),
4144
"?alias_to_cube",
4245
"?ungrouped",
46+
"?in_projection",
4347
"?cube_members",
4448
),
4549
),

‎rust/cubesql/cubesql/src/compile/rewrite/rules/wrapper/binary_expr.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,20 +20,23 @@ impl WrapperRules {
2020
binary_expr("?left", "?op", "?right"),
2121
"?alias_to_cube",
2222
"?ungrouped",
23+
"?in_projection",
2324
"?cube_members",
2425
),
2526
binary_expr(
2627
wrapper_pushdown_replacer(
2728
"?left",
2829
"?alias_to_cube",
2930
"?ungrouped",
31+
"?in_projection",
3032
"?cube_members",
3133
),
3234
"?op",
3335
wrapper_pushdown_replacer(
3436
"?right",
3537
"?alias_to_cube",
3638
"?ungrouped",
39+
"?in_projection",
3740
"?cube_members",
3841
),
3942
),
@@ -45,20 +48,23 @@ impl WrapperRules {
4548
"?left",
4649
"?alias_to_cube",
4750
"?ungrouped",
51+
"?in_projection",
4852
"?cube_members",
4953
),
5054
"?op",
5155
wrapper_pullup_replacer(
5256
"?right",
5357
"?alias_to_cube",
5458
"?ungrouped",
59+
"?in_projection",
5560
"?cube_members",
5661
),
5762
),
5863
wrapper_pullup_replacer(
5964
binary_expr("?left", "?op", "?right"),
6065
"?alias_to_cube",
6166
"?ungrouped",
67+
"?in_projection",
6268
"?cube_members",
6369
),
6470
self.transform_binary_expr("?op", "?alias_to_cube"),

‎rust/cubesql/cubesql/src/compile/rewrite/rules/wrapper/case.rs‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,25 +17,29 @@ impl WrapperRules {
1717
case_expr_var_arg("?when", "?then", "?else"),
1818
"?alias_to_cube",
1919
"?ungrouped",
20+
"?in_projection",
2021
"?cube_members",
2122
),
2223
case_expr_var_arg(
2324
wrapper_pushdown_replacer(
2425
"?when",
2526
"?alias_to_cube",
2627
"?ungrouped",
28+
"?in_projection",
2729
"?cube_members",
2830
),
2931
wrapper_pushdown_replacer(
3032
"?then",
3133
"?alias_to_cube",
3234
"?ungrouped",
35+
"?in_projection",
3336
"?cube_members",
3437
),
3538
wrapper_pushdown_replacer(
3639
"?else",
3740
"?alias_to_cube",
3841
"?ungrouped",
42+
"?in_projection",
3943
"?cube_members",
4044
),
4145
),
@@ -47,25 +51,29 @@ impl WrapperRules {
4751
"?when",
4852
"?alias_to_cube",
4953
"?ungrouped",
54+
"?in_projection",
5055
"?cube_members",
5156
),
5257
wrapper_pullup_replacer(
5358
"?then",
5459
"?alias_to_cube",
5560
"?ungrouped",
61+
"?in_projection",
5662
"?cube_members",
5763
),
5864
wrapper_pullup_replacer(
5965
"?else",
6066
"?alias_to_cube",
6167
"?ungrouped",
68+
"?in_projection",
6269
"?cube_members",
6370
),
6471
),
6572
wrapper_pullup_replacer(
6673
case_expr_var_arg("?when", "?then", "?else"),
6774
"?alias_to_cube",
6875
"?ungrouped",
76+
"?in_projection",
6977
"?cube_members",
7078
),
7179
self.transform_case_expr("?alias_to_cube"),

0 commit comments

Comments
 (0)