Skip to content

perf: compare nested ScalarValues by type before converting to ArrayData - #26066

Merged
adriangb merged 3 commits into
apache:mainfrom
alexy:nested-scalar-eq
Oct 14, 2026
Merged

adriangb merged 3 commits into
apache:mainfrom
alexy:nested-scalar-eq

Conversation

@alexy

@alexy alexy commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Physical planning slows down, faster than the query grows, when projections carry many typed NULL struct literals: a UNION ALL of 35 branches with 35 struct columns of 20 fields takes 862 ms in create_physical_plan on main. EquivalenceGroup::add_constant compares each new constant with every existing class, and ScalarValue::eq compares nested values with arrow's array equality, which converts both arrays to ArrayData before it compares anything, even when the two values have different types.

What changes are included in this PR?

ScalarValue::eq compares the nested variants (List, LargeList, FixedSizeList, ListView, LargeListView, Struct, Map) through a helper that returns early for the same Arc, and compares lengths and data types before arrow's array equality. Results are unchanged; only values of the same type and length reach the ArrayData comparison.

With the reproducer from the issue (release build, Apple M1 Max), create_physical_plan:

struct columns x fields main this PR
10 x 20 31 ms 17 ms
20 x 20 179 ms 47 ms
35 x 20 862 ms 147 ms
35 x 40 1,766 ms 253 ms

What is the testing strategy for this PR?

test_nested_scalar_eq covers typed NULL structs of the same and of different types, struct values that are equal and unequal, and lists of different element types. The scalar tests in datafusion-common pass.

Are there any user-facing changes?

No.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added the common Related to common crate label Oct 6, 2026

@adriangb adriangb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THanks!

@adriangb
adriangb enabled auto-merge October 13, 2026 21:38
Comment thread datafusion/common/src/scalar/mod.rs Outdated
Comment on lines +484 to +485
// manual implementation of `PartialEq`
/// Equality of the arrays inside nested scalars.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move // manual implementation of PartialEq` to where it was originally

Comment thread datafusion/common/src/scalar/mod.rs
Comment thread datafusion/common/src/scalar/mod.rs Outdated
@github-actions github-actions Bot added the auto detected api change Auto detected API change label Oct 14, 2026
@alamb

alamb commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

Thank you @alexy

@github-actions github-actions Bot removed the auto detected api change Auto detected API change label Oct 14, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.83%. Comparing base (b40d696) to head (6a218f4).
⚠️ Report is 108 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #26066      +/-   ##
==========================================
+ Coverage   82.69%   82.83%   +0.14%     
==========================================
  Files        1147     1149       +2     
  Lines      447204   452843    +5639     
  Branches   447204   452843    +5639     
==========================================
+ Hits       369793   375134    +5341     
+ Misses      55001    54882     -119     
- Partials    22410    22827     +417     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@adriangb
adriangb added this pull request to the merge queue Oct 14, 2026
Merged via the queue into apache:main with commit cda4dd6 Oct 14, 2026
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate v56.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Slow physical planning with many typed NULL struct literals: ScalarValue equality converts nested arrays to ArrayData

4 participants