Skip to content

Commit

Permalink
fix(cubesql): Break cost symmetry for (non)-push-to-Cube WrappedSelect (
Browse files Browse the repository at this point in the history
#9155)

Without this two different representations can have zero members, different push-to-Cube and same cost.
It could be done in a single cost component, like zero_members_wrapper, but would require to have a complex dispatch instead of add_child
  • Loading branch information
mcheshkov authored Feb 24, 2025
1 parent aba6430 commit 2c0e443
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 1 deletion.
13 changes: 12 additions & 1 deletion rust/cubesql/cubesql/src/compile/rewrite/cost.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use crate::{
compile::rewrite::{
rules::utils::granularity_str_to_int_order, CubeScanUngrouped, CubeScanWrapped,
DimensionName, LogicalPlanLanguage, MemberErrorPriority, ScalarUDFExprFun,
TimeDimensionGranularity, WrappedSelectUngroupedScan,
TimeDimensionGranularity, WrappedSelectPushToCube, WrappedSelectUngroupedScan,
},
transport::{MetaContext, V1CubeMetaDimensionExt},
};
Expand Down Expand Up @@ -189,6 +189,11 @@ impl BestCubePlan {
_ => 0,
};

let wrapped_select_non_push_to_cube = match enode {
LogicalPlanLanguage::WrappedSelectPushToCube(WrappedSelectPushToCube(false)) => 1,
_ => 0,
};

let wrapped_select_ungrouped_scan = match enode {
LogicalPlanLanguage::WrappedSelectUngroupedScan(WrappedSelectUngroupedScan(true)) => 1,
_ => 0,
Expand Down Expand Up @@ -218,6 +223,7 @@ impl BestCubePlan {
ungrouped_aggregates: 0,
wrapper_nodes,
joins,
wrapped_select_non_push_to_cube,
wrapped_select_ungrouped_scan,
empty_wrappers: 0,
ast_size_outside_wrapper: 0,
Expand All @@ -243,6 +249,7 @@ impl BestCubePlan {
/// - `member_errors` > `wrapper_nodes` - use SQL push down where possible if cube scan can't be detected
/// - `non_pushed_down_window` > `wrapper_nodes` - prefer to always push down window functions
/// - `non_pushed_down_limit_sort` > `wrapper_nodes` - prefer to always push down limit-sort expressions
/// - `wrapped_select_non_push_to_cube` > `wrapped_select_ungrouped_scan` - otherwise cost would prefer any aggregation, even non-push-to-Cube
/// - match errors by priority - optimize for more specific errors
#[derive(Debug, Clone, Ord, PartialOrd, Eq, PartialEq)]
pub struct CubePlanCost {
Expand All @@ -260,6 +267,7 @@ pub struct CubePlanCost {
joins: usize,
wrapper_nodes: i64,
ast_size_outside_wrapper: usize,
wrapped_select_non_push_to_cube: usize,
wrapped_select_ungrouped_scan: usize,
filters: i64,
structure_points: i64,
Expand Down Expand Up @@ -386,6 +394,8 @@ impl CubePlanCost {
+ other.ast_size_outside_wrapper,
ungrouped_aggregates: self.ungrouped_aggregates + other.ungrouped_aggregates,
wrapper_nodes: self.wrapper_nodes + other.wrapper_nodes,
wrapped_select_non_push_to_cube: self.wrapped_select_non_push_to_cube
+ other.wrapped_select_non_push_to_cube,
wrapped_select_ungrouped_scan: self.wrapped_select_ungrouped_scan
+ other.wrapped_select_ungrouped_scan,
cube_scan_nodes: self.cube_scan_nodes + other.cube_scan_nodes,
Expand Down Expand Up @@ -472,6 +482,7 @@ impl CubePlanCost {
} + self.ungrouped_aggregates,
unwrapped_subqueries: self.unwrapped_subqueries,
wrapper_nodes: self.wrapper_nodes,
wrapped_select_non_push_to_cube: self.wrapped_select_non_push_to_cube,
wrapped_select_ungrouped_scan: self.wrapped_select_ungrouped_scan,
cube_scan_nodes: self.cube_scan_nodes,
ast_size_without_alias: self.ast_size_without_alias,
Expand Down
38 changes: 38 additions & 0 deletions rust/cubesql/cubesql/src/compile/test/test_wrapper.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1521,3 +1521,41 @@ LIMIT 1
.sql
.contains(r#""foo" "bar""#));
}

/// Simple wrapper with cast should have explicit members, not zero
#[tokio::test]
async fn wrapper_cast_limit_explicit_members() {
if !Rewriter::sql_push_down_enabled() {
return;
}
init_testing_logger();

let query_plan = convert_select_to_query_plan(
// language=PostgreSQL
r#"
SELECT
CAST(dim_date0 AS DATE) AS "dim_date0"
FROM
MultiTypeCube
LIMIT 10
;
"#
.to_string(),
DatabaseProtocol::PostgreSQL,
)
.await;

let physical_plan = query_plan.as_physical_plan().await.unwrap();
println!(
"Physical plan: {}",
displayable(physical_plan.as_ref()).indent()
);

// Query should mention just a single member
let request = query_plan
.as_logical_plan()
.find_cube_scan_wrapped_sql()
.request;
assert_eq!(request.measures.unwrap().len(), 1);
assert_eq!(request.dimensions.unwrap().len(), 0);
}

0 comments on commit 2c0e443

Please sign in to comment.