From f5a7cb25610182ed448e08d22a883da825484118 Mon Sep 17 00:00:00 2001 From: Yongting You <2010youy01@gmail.com> Date: Mon, 31 Aug 2026 03:08:40 +0000 Subject: [PATCH 1/2] Minor: collapse duplicated branches that differ by one value Several places build the same value twice in `if`/`else` arms where only one field, argument or fragment differs. Compute the differing part in the branch and build the value once. - `to_proto` / `unparser::expr`: `ILikeNode`/`LikeNode` and `ast::Expr::ILike`/`ast::Expr::Like` were built from identical field lists. Serialize the operands once, then pick the node type. - `LogicalPlanBuilder::intersect_or_except`: the two builder chains differed only by an inserted `.distinct()?`. - `min_max_struct`: pick the comparator fn pointer in the `if`, then call `update_batch` once. - `unparser::plan`: `extension_to_sql` was called twice; only the `query` argument differed, and it is just `query.as_mut()`. - `Expr` BETWEEN display: `SchemaDisplay` and `SqlDisplay` each duplicated the format string per `negated`; a `"NOT "` fragment collapses both. - `sum`: the `helper!` macro was defined twice; only the accumulator type differed. - `hash_join` test helper: choose both option values in the `if`, then assign once. - `graphviz::add_node`: two `writeln!`s differing only by the optional tooltip. No functional change intended. --- datafusion/common/src/display/graphviz.rs | 27 ++++------ datafusion/expr/src/expr.rs | 50 ++++++------------- datafusion/expr/src/logical_plan/builder.rs | 33 +++++------- .../src/min_max/min_max_struct.rs | 20 ++------ datafusion/functions-aggregate/src/sum.rs | 25 ++++------ .../physical-plan/src/joins/hash_join/exec.rs | 29 ++++------- datafusion/proto/src/logical_plan/to_proto.rs | 39 ++++++++------- datafusion/sql/src/unparser/expr.rs | 24 +++++---- datafusion/sql/src/unparser/plan.rs | 22 +++----- 9 files changed, 106 insertions(+), 163 deletions(-) diff --git a/datafusion/common/src/display/graphviz.rs b/datafusion/common/src/display/graphviz.rs index f84490cd3ea4e..3fc9909c60c12 100644 --- a/datafusion/common/src/display/graphviz.rs +++ b/datafusion/common/src/display/graphviz.rs @@ -73,22 +73,17 @@ impl GraphvizBuilder { label: &str, tooltip: Option<&str>, ) -> fmt::Result { - if let Some(tooltip) = tooltip { - writeln!( - f, - " {}[shape=box label={}, tooltip={}]", - id, - GraphvizBuilder::quoted(label), - GraphvizBuilder::quoted(tooltip), - ) - } else { - writeln!( - f, - " {}[shape=box label={}]", - id, - GraphvizBuilder::quoted(label), - ) - } + let tooltip = tooltip + .map(|tooltip| format!(", tooltip={}", GraphvizBuilder::quoted(tooltip))) + .unwrap_or_default(); + + writeln!( + f, + " {}[shape=box label={}{}]", + id, + GraphvizBuilder::quoted(label), + tooltip, + ) } pub fn add_edge( diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs index f9c0662e682e8..e403a33e35148 100644 --- a/datafusion/expr/src/expr.rs +++ b/datafusion/expr/src/expr.rs @@ -3023,23 +3023,14 @@ impl Display for SchemaDisplay<'_> { low, high, }) => { - if *negated { - write!( - f, - "{} NOT BETWEEN {} AND {}", - SchemaDisplay(expr), - SchemaDisplay(low), - SchemaDisplay(high), - ) - } else { - write!( - f, - "{} BETWEEN {} AND {}", - SchemaDisplay(expr), - SchemaDisplay(low), - SchemaDisplay(high), - ) - } + let not = if *negated { "NOT " } else { "" }; + write!( + f, + "{} {not}BETWEEN {} AND {}", + SchemaDisplay(expr), + SchemaDisplay(low), + SchemaDisplay(high), + ) } Expr::BinaryExpr(BinaryExpr { left, op, right }) => { write!(f, "{} {op} {}", SchemaDisplay(left), SchemaDisplay(right),) @@ -3305,23 +3296,14 @@ impl Display for SqlDisplay<'_> { low, high, }) => { - if *negated { - write!( - f, - "{} NOT BETWEEN {} AND {}", - SqlDisplay(expr), - SqlDisplay(low), - SqlDisplay(high), - ) - } else { - write!( - f, - "{} BETWEEN {} AND {}", - SqlDisplay(expr), - SqlDisplay(low), - SqlDisplay(high), - ) - } + let not = if *negated { "NOT " } else { "" }; + write!( + f, + "{} {not}BETWEEN {} AND {}", + SqlDisplay(expr), + SqlDisplay(low), + SqlDisplay(high), + ) } Expr::BinaryExpr(BinaryExpr { left, op, right }) => { write!(f, "{} {op} {}", SqlDisplay(left), SqlDisplay(right),) diff --git a/datafusion/expr/src/logical_plan/builder.rs b/datafusion/expr/src/logical_plan/builder.rs index ef5e496b0d7de..d8b74fa82aa99 100644 --- a/datafusion/expr/src/logical_plan/builder.rs +++ b/datafusion/expr/src/logical_plan/builder.rs @@ -1423,28 +1423,19 @@ impl LogicalPlanBuilder { ) }) .unzip(); - if is_all { - LogicalPlanBuilder::from(left_plan) - .join_detailed( - right_plan, - join_type, - join_keys, - None, - NullEquality::NullEqualsNull, - )? - .build() - } else { - LogicalPlanBuilder::from(left_plan) - .distinct()? - .join_detailed( - right_plan, - join_type, - join_keys, - None, - NullEquality::NullEqualsNull, - )? - .build() + let mut left_builder = LogicalPlanBuilder::from(left_plan); + if !is_all { + left_builder = left_builder.distinct()?; } + left_builder + .join_detailed( + right_plan, + join_type, + join_keys, + None, + NullEquality::NullEqualsNull, + )? + .build() } /// Build the plan diff --git a/datafusion/functions-aggregate/src/min_max/min_max_struct.rs b/datafusion/functions-aggregate/src/min_max/min_max_struct.rs index 2c08a8e45d2c6..8058eb641bd61 100644 --- a/datafusion/functions-aggregate/src/min_max/min_max_struct.rs +++ b/datafusion/functions-aggregate/src/min_max/min_max_struct.rs @@ -83,21 +83,11 @@ impl GroupsAccumulator for MinMaxStructAccumulator { matches!(partial_cmp_struct(a, b), Some(Ordering::Greater)) } - if self.is_min { - self.inner.update_batch( - array.as_struct(), - group_indices, - total_num_groups, - struct_min, - ) - } else { - self.inner.update_batch( - array.as_struct(), - group_indices, - total_num_groups, - struct_max, - ) - } + let cmp: fn(&StructArray, &StructArray) -> bool = + if self.is_min { struct_min } else { struct_max }; + + self.inner + .update_batch(array.as_struct(), group_indices, total_num_groups, cmp) } fn evaluate(&mut self, emit_to: EmitTo) -> Result { diff --git a/datafusion/functions-aggregate/src/sum.rs b/datafusion/functions-aggregate/src/sum.rs index 1999f68d4cdd7..1b9660dc89b7e 100644 --- a/datafusion/functions-aggregate/src/sum.rs +++ b/datafusion/functions-aggregate/src/sum.rs @@ -261,21 +261,18 @@ impl AggregateUDFImpl for Sum { } fn accumulator(&self, args: AccumulatorArgs) -> Result> { - if args.is_distinct { - macro_rules! helper { - ($t:ty, $dt:expr) => { - Ok(Box::new(DistinctSumAccumulator::<$t>::new($dt))) - }; - } - downcast_sum!(args, helper) - } else { - macro_rules! helper { - ($t:ty, $dt:expr) => { - Ok(Box::new(SumAccumulator::<$t>::new($dt))) - }; - } - downcast_sum!(args, helper) + let is_distinct = args.is_distinct; + macro_rules! helper { + ($t:ty, $dt:expr) => { + if is_distinct { + Ok(Box::new(DistinctSumAccumulator::<$t>::new($dt)) + as Box) + } else { + Ok(Box::new(SumAccumulator::<$t>::new($dt)) as Box) + } + }; } + downcast_sum!(args, helper) } fn state_fields(&self, args: StateFieldsArgs) -> Result> { diff --git a/datafusion/physical-plan/src/joins/hash_join/exec.rs b/datafusion/physical-plan/src/joins/hash_join/exec.rs index 1b36dabebc564..054877cfd63ff 100644 --- a/datafusion/physical-plan/src/joins/hash_join/exec.rs +++ b/datafusion/physical-plan/src/joins/hash_join/exec.rs @@ -2964,25 +2964,16 @@ mod tests { ) -> Arc { let mut session_config = SessionConfig::default().with_batch_size(batch_size); - if use_perfect_hash_join_as_possible { - session_config - .options_mut() - .execution - .perfect_hash_join_small_build_threshold = 819200; - session_config - .options_mut() - .execution - .perfect_hash_join_min_key_density = 0.0; - } else { - session_config - .options_mut() - .execution - .perfect_hash_join_small_build_threshold = 0; - session_config - .options_mut() - .execution - .perfect_hash_join_min_key_density = f64::INFINITY; - } + // Either always take the perfect hash join path, or never take it. + let (small_build_threshold, min_key_density) = + if use_perfect_hash_join_as_possible { + (819200, 0.0) + } else { + (0, f64::INFINITY) + }; + let execution = &mut session_config.options_mut().execution; + execution.perfect_hash_join_small_build_threshold = small_build_threshold; + execution.perfect_hash_join_min_key_density = min_key_density; Arc::new(TaskContext::default().with_session_config(session_config)) } diff --git a/datafusion/proto/src/logical_plan/to_proto.rs b/datafusion/proto/src/logical_plan/to_proto.rs index 16c3468465541..ef49b46d1a04e 100644 --- a/datafusion/proto/src/logical_plan/to_proto.rs +++ b/datafusion/proto/src/logical_plan/to_proto.rs @@ -126,28 +126,29 @@ pub fn serialize_expr( escape_char, case_insensitive, }) => { - if *case_insensitive { - let pb = Box::new(protobuf::ILikeNode { - negated: *negated, - expr: Some(Box::new(serialize_expr(expr.as_ref(), codec)?)), - pattern: Some(Box::new(serialize_expr(pattern.as_ref(), codec)?)), - escape_char: escape_char.map(|ch| ch.to_string()).unwrap_or_default(), - }); + let negated = *negated; + let expr = Some(Box::new(serialize_expr(expr.as_ref(), codec)?)); + let pattern = Some(Box::new(serialize_expr(pattern.as_ref(), codec)?)); + let escape_char = escape_char.map(|ch| ch.to_string()).unwrap_or_default(); - protobuf::LogicalExprNode { - expr_type: Some(ExprType::Ilike(pb)), - } + let expr_type = if *case_insensitive { + ExprType::Ilike(Box::new(protobuf::ILikeNode { + negated, + expr, + pattern, + escape_char, + })) } else { - let pb = Box::new(protobuf::LikeNode { - negated: *negated, - expr: Some(Box::new(serialize_expr(expr.as_ref(), codec)?)), - pattern: Some(Box::new(serialize_expr(pattern.as_ref(), codec)?)), - escape_char: escape_char.map(|ch| ch.to_string()).unwrap_or_default(), - }); + ExprType::Like(Box::new(protobuf::LikeNode { + negated, + expr, + pattern, + escape_char, + })) + }; - protobuf::LogicalExprNode { - expr_type: Some(ExprType::Like(pb)), - } + protobuf::LogicalExprNode { + expr_type: Some(expr_type), } } Expr::SimilarTo(Like { diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs index 898330018c708..baa64da7733de 100644 --- a/datafusion/sql/src/unparser/expr.rs +++ b/datafusion/sql/src/unparser/expr.rs @@ -371,22 +371,26 @@ impl Unparser<'_> { escape_char, case_insensitive, }) => { + let negated = *negated; + let expr = Box::new(self.expr_to_sql_inner(expr)?); + let pattern = Box::new(self.expr_to_sql_inner(pattern)?); + let escape_char = + escape_char.map(|c| SingleQuotedString(c.to_string()).into()); + if *case_insensitive { Ok(ast::Expr::ILike { - negated: *negated, - expr: Box::new(self.expr_to_sql_inner(expr)?), - pattern: Box::new(self.expr_to_sql_inner(pattern)?), - escape_char: escape_char - .map(|c| SingleQuotedString(c.to_string()).into()), + negated, + expr, + pattern, + escape_char, any: false, }) } else { Ok(ast::Expr::Like { - negated: *negated, - expr: Box::new(self.expr_to_sql_inner(expr)?), - pattern: Box::new(self.expr_to_sql_inner(pattern)?), - escape_char: escape_char - .map(|c| SingleQuotedString(c.to_string()).into()), + negated, + expr, + pattern, + escape_char, any: false, }) } diff --git a/datafusion/sql/src/unparser/plan.rs b/datafusion/sql/src/unparser/plan.rs index 9922509a0e609..d38b1432913b1 100644 --- a/datafusion/sql/src/unparser/plan.rs +++ b/datafusion/sql/src/unparser/plan.rs @@ -1675,21 +1675,13 @@ impl Unparser<'_> { Ok(()) } LogicalPlan::Extension(extension) => { - if let Some(query) = query.as_mut() { - self.extension_to_sql( - extension.node.as_ref(), - &mut Some(query), - &mut Some(select), - &mut Some(relation), - ) - } else { - self.extension_to_sql( - extension.node.as_ref(), - &mut None, - &mut Some(select), - &mut Some(relation), - ) - } + let mut query = query.as_mut(); + self.extension_to_sql( + extension.node.as_ref(), + &mut query, + &mut Some(select), + &mut Some(relation), + ) } LogicalPlan::Unnest(unnest) => { if !unnest.struct_type_columns.is_empty() { From 67301444cf1973ae42ecc7afed1849278f5175c4 Mon Sep 17 00:00:00 2001 From: Yongting You <2010youy01@gmail.com> Date: Mon, 31 Aug 2026 21:26:49 +0800 Subject: [PATCH 2/2] review --- .../src/min_max/min_max_struct.rs | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/datafusion/functions-aggregate/src/min_max/min_max_struct.rs b/datafusion/functions-aggregate/src/min_max/min_max_struct.rs index 8058eb641bd61..2c08a8e45d2c6 100644 --- a/datafusion/functions-aggregate/src/min_max/min_max_struct.rs +++ b/datafusion/functions-aggregate/src/min_max/min_max_struct.rs @@ -83,11 +83,21 @@ impl GroupsAccumulator for MinMaxStructAccumulator { matches!(partial_cmp_struct(a, b), Some(Ordering::Greater)) } - let cmp: fn(&StructArray, &StructArray) -> bool = - if self.is_min { struct_min } else { struct_max }; - - self.inner - .update_batch(array.as_struct(), group_indices, total_num_groups, cmp) + if self.is_min { + self.inner.update_batch( + array.as_struct(), + group_indices, + total_num_groups, + struct_min, + ) + } else { + self.inner.update_batch( + array.as_struct(), + group_indices, + total_num_groups, + struct_max, + ) + } } fn evaluate(&mut self, emit_to: EmitTo) -> Result {