chore: collapse duplicated branches that differ by one value - #24814
Open
2010YOUY01 wants to merge 2 commits into
Open
chore: collapse duplicated branches that differ by one value#248142010YOUY01 wants to merge 2 commits into
2010YOUY01 wants to merge 2 commits into
Conversation
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.
github-actions
Bot
added
sql
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #24814 +/- ##
==========================================
+ Coverage 81.52% 81.58% +0.05%
==========================================
Files 1123 1123
Lines 406148 406590 +442
Branches 406148 406590 +442
==========================================
+ Hits 331124 331701 +577
+ Misses 55659 55453 -206
- Partials 19365 19436 +71 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
getChan
reviewed
Aug 31, 2026
Comment on lines
62
to
93
| } | ||
|
|
||
| impl GroupsAccumulator for MinMaxStructAccumulator { | ||
| fn update_batch( | ||
| &mut self, | ||
| values: &[ArrayRef], | ||
| group_indices: &[usize], | ||
| opt_filter: Option<&BooleanArray>, | ||
| total_num_groups: usize, | ||
| ) -> Result<()> { | ||
| let array = &values[0]; | ||
| assert_eq!(array.len(), group_indices.len()); | ||
| assert_eq!(array.data_type(), &self.inner.data_type); | ||
| // apply filter if needed | ||
| let array = apply_filter_as_nulls(array, opt_filter)?; | ||
|
|
||
| fn struct_min(a: &StructArray, b: &StructArray) -> bool { | ||
| matches!(partial_cmp_struct(a, b), Some(Ordering::Less)) | ||
| } | ||
|
|
||
| fn struct_max(a: &StructArray, b: &StructArray) -> bool { | ||
| 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<ArrayRef> { |
Contributor
There was a problem hiding this comment.
nit: This refactoring coerces struct_min / struct_max to a function pointer. Since cmp is called from the per-row loop in MinMaxStructState::update_batch, this is a hot path where even small overhead can accumulate.
I compared the release LLVM IR locally: the updated version selects a function pointer and invokes it indirectly inside the loop, whereas the original version produces separate MIN/MAX loops. I also ran a small local release benchmark for a single-field STRUCT MAX aggregation; the median changed from 1.20s to 1.46s (+22%).
The original code is a little more verbose, but would you mind keeping the two branches here to preserve static dispatch?
Contributor
Author
There was a problem hiding this comment.
good catch, reverted
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
The following code snippet demonstrates the refactor idea, I find the second version is more eye-friendly, especially when the struct gets large.
This PR applies this idea to safe locations across the codebase.
What changes are included in this PR?
What is the testing strategy for this PR?
Are there any user-facing changes?