Skip to content

Commit ab5a64e

Browse files
authored
fix(cubesql): Use pushdown-pullup scheme for FilterSimplifyReplacer (#9278)
This should avoid unexpected unifications of simplified expression in unrelated context. For example, same expression can be present in filter and projection, but due to over-unification in filters it can receive different representation in projection, and break aliases later, during extraction Supporting changes: * Remove unnecessary Vec+Box for rules * Add NOW function template A couple of unit tests with NOW and wrapper were relying on over-unification, and broke without a function template. * Bump default rewrite iteration limit to 500 With new rules `select_many_filters` benchmark was hitting iteration limit
1 parent 6f43138 commit ab5a64e

File tree

7 files changed

+382
-138
lines changed

7 files changed

+382
-138
lines changed

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

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12878,7 +12878,7 @@ ORDER BY "source"."str0" ASC
1287812878
}
1287912879
init_testing_logger();
1288012880

12881-
let logical_plan = convert_select_to_query_plan(
12881+
let query_plan = convert_select_to_query_plan(
1288212882
r#"
1288312883
SELECT
1288412884
CAST("public"."KibanaSampleDataEcommerce"."order_date" AS DATE) AS "order_date",
@@ -12899,11 +12899,16 @@ ORDER BY "source"."str0" ASC
1289912899
.to_string(),
1290012900
DatabaseProtocol::PostgreSQL,
1290112901
)
12902-
.await
12903-
.as_logical_plan();
12902+
.await;
12903+
12904+
let physical_plan = query_plan.as_physical_plan().await.unwrap();
12905+
println!(
12906+
"Physical plan: {}",
12907+
displayable(physical_plan.as_ref()).indent()
12908+
);
12909+
12910+
let logical_plan = query_plan.as_logical_plan();
1290412911

12905-
let end_date = chrono::Utc::now().date_naive();
12906-
let start_date = end_date - chrono::Duration::days(30);
1290712912
assert_eq!(
1290812913
logical_plan.find_cube_scan_wrapped_sql().request,
1290912914
V1LoadRequestQuery {
@@ -12930,7 +12935,7 @@ ORDER BY "source"."str0" ASC
1293012935
"cube_name": "KibanaSampleDataEcommerce",
1293112936
"alias": "kibanasampledata",
1293212937
"cube_params": ["KibanaSampleDataEcommerce"],
12933-
"expr": format!("(((${{KibanaSampleDataEcommerce.order_date}} >= DATE('{start_date}')) AND (${{KibanaSampleDataEcommerce.order_date}} < DATE('{end_date}'))) AND (((${{KibanaSampleDataEcommerce.notes}} = $0$) OR (${{KibanaSampleDataEcommerce.notes}} = $1$)) OR (${{KibanaSampleDataEcommerce.notes}} = $2$)))"),
12938+
"expr": format!("(((${{KibanaSampleDataEcommerce.order_date}} >= CAST((NOW() + INTERVAL '-30 DAY') AS DATE)) AND (${{KibanaSampleDataEcommerce.order_date}} < CAST(NOW() AS DATE))) AND (((${{KibanaSampleDataEcommerce.notes}} = $0$) OR (${{KibanaSampleDataEcommerce.notes}} = $1$)) OR (${{KibanaSampleDataEcommerce.notes}} = $2$)))"),
1293412939
"grouping_set": null,
1293512940
}).to_string(),
1293612941
]),

rust/cubesql/cubesql/src/compile/rewrite/cost.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,8 @@ impl BestCubePlan {
104104
LogicalPlanLanguage::OrderReplacer(_) => 1,
105105
LogicalPlanLanguage::MemberReplacer(_) => 1,
106106
LogicalPlanLanguage::FilterReplacer(_) => 1,
107+
LogicalPlanLanguage::FilterSimplifyPushDownReplacer(_) => 1,
108+
LogicalPlanLanguage::FilterSimplifyPullUpReplacer(_) => 1,
107109
LogicalPlanLanguage::TimeDimensionDateRangeReplacer(_) => 1,
108110
LogicalPlanLanguage::InnerAggregateSplitReplacer(_) => 1,
109111
LogicalPlanLanguage::OuterProjectionSplitReplacer(_) => 1,

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,10 @@ crate::plan_to_language! {
405405
members: Vec<LogicalPlan>,
406406
aliases: Vec<(String, String)>,
407407
},
408-
FilterSimplifyReplacer {
408+
FilterSimplifyPushDownReplacer {
409+
filters: Vec<LogicalPlan>,
410+
},
411+
FilterSimplifyPullUpReplacer {
409412
filters: Vec<LogicalPlan>,
410413
},
411414
OrderReplacer {
@@ -1901,8 +1904,12 @@ fn filter_replacer(
19011904
)
19021905
}
19031906

1904-
fn filter_simplify_replacer(members: impl Display) -> String {
1905-
format!("(FilterSimplifyReplacer {})", members)
1907+
fn filter_simplify_push_down_replacer(members: impl Display) -> String {
1908+
format!("(FilterSimplifyPushDownReplacer {})", members)
1909+
}
1910+
1911+
fn filter_simplify_pull_up_replacer(members: impl Display) -> String {
1912+
format!("(FilterSimplifyPullUpReplacer {})", members)
19061913
}
19071914

19081915
fn inner_aggregate_split_replacer(members: impl Display, alias_to_cube: impl Display) -> String {

rust/cubesql/cubesql/src/compile/rewrite/rewriter.rs

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -208,7 +208,7 @@ impl Rewriter {
208208
.with_iter_limit(
209209
env::var("CUBESQL_REWRITE_MAX_ITERATIONS")
210210
.map(|v| v.parse::<usize>().unwrap())
211-
.unwrap_or(300),
211+
.unwrap_or(500),
212212
)
213213
.with_node_limit(
214214
env::var("CUBESQL_REWRITE_MAX_NODES")
@@ -473,20 +473,16 @@ impl Rewriter {
473473
eval_stable_functions: bool,
474474
) -> Vec<CubeRewrite> {
475475
let sql_push_down = Self::sql_push_down_enabled();
476-
let rules: Vec<Box<dyn RewriteRules>> = vec![
477-
Box::new(MemberRules::new(
478-
meta_context.clone(),
479-
config_obj.clone(),
480-
sql_push_down,
481-
)),
482-
Box::new(FilterRules::new(
476+
let rules: &[&dyn RewriteRules] = &[
477+
&MemberRules::new(meta_context.clone(), config_obj.clone(), sql_push_down),
478+
&FilterRules::new(
483479
meta_context.clone(),
484480
config_obj.clone(),
485481
eval_stable_functions,
486-
)),
487-
Box::new(DateRules::new(config_obj.clone())),
488-
Box::new(OrderRules::new()),
489-
Box::new(CommonRules::new(config_obj.clone())),
482+
),
483+
&DateRules::new(config_obj.clone()),
484+
&OrderRules::new(),
485+
&CommonRules::new(config_obj.clone()),
490486
];
491487
let mut rewrites = Vec::new();
492488
for r in rules {

0 commit comments

Comments
 (0)