Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some old feature flags around window functions #30641

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/adapter/src/optimize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,10 +251,6 @@ impl From<&OptimizerConfig> for mz_sql::plan::HirToMirConfig {
Self {
enable_new_outer_join_lowering: config.features.enable_new_outer_join_lowering,
enable_variadic_left_join_lowering: config.features.enable_variadic_left_join_lowering,
enable_value_window_function_fusion: config
.features
.enable_value_window_function_fusion,
enable_window_aggregation_fusion: config.features.enable_window_aggregation_fusion,
}
}
}
Expand Down
13 changes: 3 additions & 10 deletions src/compute-types/src/plan/lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ pub(super) struct Context {
debug_info: LirDebugInfo,
/// Whether to enable fusion of MFPs in reductions.
enable_reduce_mfp_fusion: bool,
/// Whether to fuse `Reduce` with `FlatMap UnnestList` for better window function performance.
enable_reduce_unnest_list_fusion: bool,
}

impl Context {
Expand All @@ -51,7 +49,6 @@ impl Context {
id: GlobalId::Transient(0),
},
enable_reduce_mfp_fusion: features.enable_reduce_mfp_fusion,
enable_reduce_unnest_list_fusion: features.enable_reduce_unnest_list_fusion,
}
}

Expand Down Expand Up @@ -418,9 +415,6 @@ impl Context {
// it would be very hard to hunt down all these parts. (For example, key inference
// infers the group key as a unique key.)
let fused_with_reduce = 'fusion: {
if !self.enable_reduce_unnest_list_fusion {
break 'fusion None;
}
if !matches!(func, TableFunc::UnnestList { .. }) {
break 'fusion None;
}
Expand Down Expand Up @@ -692,10 +686,9 @@ This is not expected to cause incorrect results, but could indicate a performanc
monotonic,
expected_group_size,
} => {
if self.enable_reduce_unnest_list_fusion
&& aggregates
.iter()
.any(|agg| agg.func.can_fuse_with_unnest_list())
if aggregates
.iter()
.any(|agg| agg.func.can_fuse_with_unnest_list())
{
// This case should have been handled at the `MirRelationExpr::FlatMap` case
// above. But that has a pretty complicated pattern matching, so it's not
Expand Down
6 changes: 0 additions & 6 deletions src/repr/src/optimize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,12 +116,6 @@ optimizer_feature_flags!({
// Reoptimize imported views when building and optimizing a
// `DataflowDescription` in the global MIR optimization phase.
reoptimize_imported_views: bool,
// Enables the value window function fusion optimization.
enable_value_window_function_fusion: bool,
// See the feature flag of the same name.
enable_reduce_unnest_list_fusion: bool,
// See the feature flag of the same name.
enable_window_aggregation_fusion: bool,
// See the feature flag of the same name.
enable_reduce_reduction: bool,
});
Expand Down
16 changes: 2 additions & 14 deletions src/sql-parser/src/ast/defs/statement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2328,9 +2328,6 @@ pub enum ClusterFeatureName {
EnableEagerDeltaJoins,
EnableVariadicLeftJoinLowering,
EnableLetrecFixpointAnalysis,
EnableValueWindowFunctionFusion,
EnableReduceUnnestListFusion,
EnableWindowAggregationFusion,
}

impl WithOptionName for ClusterFeatureName {
Expand All @@ -2345,10 +2342,7 @@ impl WithOptionName for ClusterFeatureName {
| Self::EnableNewOuterJoinLowering
| Self::EnableEagerDeltaJoins
| Self::EnableVariadicLeftJoinLowering
| Self::EnableLetrecFixpointAnalysis
| Self::EnableValueWindowFunctionFusion
| Self::EnableReduceUnnestListFusion
| Self::EnableWindowAggregationFusion => false,
| Self::EnableLetrecFixpointAnalysis => false,
}
}
}
Expand Down Expand Up @@ -3894,9 +3888,6 @@ pub enum ExplainPlanOptionName {
EnableEagerDeltaJoins,
EnableVariadicLeftJoinLowering,
EnableLetrecFixpointAnalysis,
EnableValueWindowFunctionFusion,
EnableReduceUnnestListFusion,
EnableWindowAggregationFusion,
}

impl WithOptionName for ExplainPlanOptionName {
Expand Down Expand Up @@ -3930,10 +3921,7 @@ impl WithOptionName for ExplainPlanOptionName {
| Self::EnableNewOuterJoinLowering
| Self::EnableEagerDeltaJoins
| Self::EnableVariadicLeftJoinLowering
| Self::EnableLetrecFixpointAnalysis
| Self::EnableValueWindowFunctionFusion
| Self::EnableReduceUnnestListFusion
| Self::EnableWindowAggregationFusion => false,
| Self::EnableLetrecFixpointAnalysis => false,
}
}
}
Expand Down
6 changes: 0 additions & 6 deletions src/sql/src/plan/lowering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,19 +136,13 @@ pub struct Config {
pub enable_new_outer_join_lowering: bool,
/// Enable outer join lowering implemented in database-issues#7561.
pub enable_variadic_left_join_lowering: bool,
/// See the feature flag of the same name.
pub enable_value_window_function_fusion: bool,
/// See the feature flag of the same name.
pub enable_window_aggregation_fusion: bool,
}

impl From<&SystemVars> for Config {
fn from(vars: &SystemVars) -> Self {
Self {
enable_new_outer_join_lowering: vars.enable_new_outer_join_lowering(),
enable_variadic_left_join_lowering: vars.enable_variadic_left_join_lowering(),
enable_value_window_function_fusion: vars.enable_value_window_function_fusion(),
enable_window_aggregation_fusion: vars.enable_window_aggregation_fusion(),
}
}
}
Expand Down
17 changes: 1 addition & 16 deletions src/sql/src/plan/statement/ddl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4414,10 +4414,7 @@ generate_extracted_config!(
(EnableEagerDeltaJoins, Option<bool>, Default(None)),
(EnableNewOuterJoinLowering, Option<bool>, Default(None)),
(EnableVariadicLeftJoinLowering, Option<bool>, Default(None)),
(EnableLetrecFixpointAnalysis, Option<bool>, Default(None)),
(EnableValueWindowFunctionFusion, Option<bool>, Default(None)),
(EnableReduceUnnestListFusion, Option<bool>, Default(None)),
(EnableWindowAggregationFusion, Option<bool>, Default(None))
(EnableLetrecFixpointAnalysis, Option<bool>, Default(None))
);

/// Convert a [`CreateClusterStatement`] into a [`Plan`].
Expand Down Expand Up @@ -4554,9 +4551,6 @@ pub fn plan_create_cluster_inner(
enable_new_outer_join_lowering,
enable_variadic_left_join_lowering,
enable_letrec_fixpoint_analysis,
enable_value_window_function_fusion,
enable_reduce_unnest_list_fusion,
enable_window_aggregation_fusion,
seen: _,
} = ClusterFeatureExtracted::try_from(features)?;
let optimizer_feature_overrides = OptimizerFeatureOverrides {
Expand All @@ -4565,9 +4559,6 @@ pub fn plan_create_cluster_inner(
enable_new_outer_join_lowering,
enable_variadic_left_join_lowering,
enable_letrec_fixpoint_analysis,
enable_value_window_function_fusion,
enable_reduce_unnest_list_fusion,
enable_window_aggregation_fusion,
..Default::default()
};

Expand Down Expand Up @@ -4662,9 +4653,6 @@ pub fn unplan_create_cluster(
enable_new_outer_join_lowering,
enable_variadic_left_join_lowering,
enable_letrec_fixpoint_analysis,
enable_value_window_function_fusion,
enable_reduce_unnest_list_fusion,
enable_window_aggregation_fusion,
enable_reduce_reduction: _,
} = optimizer_feature_overrides;
let features_extracted = ClusterFeatureExtracted {
Expand All @@ -4675,9 +4663,6 @@ pub fn unplan_create_cluster(
enable_new_outer_join_lowering,
enable_variadic_left_join_lowering,
enable_letrec_fixpoint_analysis,
enable_value_window_function_fusion,
enable_reduce_unnest_list_fusion,
enable_window_aggregation_fusion,
};
let features = features_extracted.into_values(scx.catalog);
let availability_zones = if availability_zones.is_empty() {
Expand Down
8 changes: 1 addition & 7 deletions src/sql/src/plan/statement/dml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,10 +368,7 @@ generate_extracted_config!(
(EnableNewOuterJoinLowering, Option<bool>, Default(None)),
(EnableEagerDeltaJoins, Option<bool>, Default(None)),
(EnableVariadicLeftJoinLowering, Option<bool>, Default(None)),
(EnableLetrecFixpointAnalysis, Option<bool>, Default(None)),
(EnableValueWindowFunctionFusion, Option<bool>, Default(None)),
(EnableReduceUnnestListFusion, Option<bool>, Default(None)),
(EnableWindowAggregationFusion, Option<bool>, Default(None))
(EnableLetrecFixpointAnalysis, Option<bool>, Default(None))
);

impl TryFrom<ExplainPlanOptionExtracted> for ExplainConfig {
Expand Down Expand Up @@ -422,9 +419,6 @@ impl TryFrom<ExplainPlanOptionExtracted> for ExplainConfig {
enable_cardinality_estimates: Default::default(),
persist_fast_path_limit: Default::default(),
reoptimize_imported_views: v.reoptimize_imported_views,
enable_value_window_function_fusion: v.enable_value_window_function_fusion,
enable_reduce_unnest_list_fusion: v.enable_reduce_unnest_list_fusion,
enable_window_aggregation_fusion: v.enable_window_aggregation_fusion,
enable_reduce_reduction: Default::default(),
},
})
Expand Down
14 changes: 2 additions & 12 deletions src/sql/src/plan/transform_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,14 +353,8 @@ fn column_type(
/// columns in the group.)
pub fn fuse_window_functions(
root: &mut HirRelationExpr,
context: &crate::plan::lowering::Context,
_context: &crate::plan::lowering::Context,
) -> Result<(), RecursionLimitError> {
if !(context.config.enable_value_window_function_fusion
|| context.config.enable_window_aggregation_fusion)
{
return Ok(());
}

impl HirScalarExpr {
/// Similar to `MirScalarExpr::support`, but adapted to `HirScalarExpr` in a special way: it
/// considers column references that target the root level.
Expand Down Expand Up @@ -589,7 +583,6 @@ pub fn fuse_window_functions(
// encounter these, because we just do one pass, but it's better to be
// robust against future code changes.)
!matches!(func, ValueWindowFunc::Fused(..))
&& context.config.enable_value_window_function_fusion
}
HirScalarExpr::Windowing(WindowExpr {
func:
Expand All @@ -598,10 +591,7 @@ pub fn fuse_window_functions(
..
}),
..
}) => {
!matches!(func, AggregateFunc::FusedWindowAgg { .. })
&& context.config.enable_window_aggregation_fusion
}
}) => !matches!(func, AggregateFunc::FusedWindowAgg { .. }),
_ => false,
}
};
Expand Down
21 changes: 0 additions & 21 deletions src/sql/src/session/vars/definitions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2176,24 +2176,6 @@ feature_flags!(
default: false,
enable_for_item_parsing: false,
},
{
name: enable_value_window_function_fusion,
desc: "Enables the value window function fusion optimization",
default: true,
enable_for_item_parsing: false,
},
{
name: enable_window_aggregation_fusion,
desc: "Enables the window aggregation fusion optimization",
default: true,
enable_for_item_parsing: false,
},
{
name: enable_reduce_unnest_list_fusion,
desc: "Enables fusing `Reduce` with `FlatMap UnnestList` for better window function performance",
default: true,
enable_for_item_parsing: false,
},
{
name: enable_continual_task_create,
desc: "CREATE CONTINUAL TASK",
Expand Down Expand Up @@ -2230,9 +2212,6 @@ impl From<&super::SystemVars> for OptimizerFeatures {
enable_variadic_left_join_lowering: vars.enable_variadic_left_join_lowering(),
enable_letrec_fixpoint_analysis: vars.enable_letrec_fixpoint_analysis(),
enable_cardinality_estimates: vars.enable_cardinality_estimates(),
enable_value_window_function_fusion: vars.enable_value_window_function_fusion(),
enable_reduce_unnest_list_fusion: vars.enable_reduce_unnest_list_fusion(),
enable_window_aggregation_fusion: vars.enable_window_aggregation_fusion(),
enable_reduce_reduction: vars.enable_reduce_reduction(),
persist_fast_path_limit: vars.persist_fast_path_limit(),
reoptimize_imported_views: false,
Expand Down
Loading