Skip to content

perf: optimize get_field - #23537

Merged
andygrove merged 2 commits into
apache:mainfrom
andygrove:auto-opt/get_field-datafusion-20260713-142422
Jul 17, 2026
Merged

perf: optimize get_field#23537
andygrove merged 2 commits into
apache:mainfrom
andygrove:auto-opt/get_field-datafusion-20260713-142422

Conversation

@andygrove

@andygrove andygrove commented Jul 13, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

N/A

Rationale for this change

Optimize existing function.

What changes are included in this PR?

Removed the per-row BooleanArray::slice allocation in map key lookup by scanning the key-comparison bitmap in place over each row's offset range.

Are these changes tested?

Existing tests.

Benchmark (criterion):

  • get_field_map_1024_entries_16_last: 6.23% faster (base 42011ns -> cand 39394ns)
  • get_field_map_1024_entries_4_missing: 12.267% faster (base 19070ns -> cand 16730ns)
  • get_field_map_8192_entries_4_last: 8.122% faster (base 168450ns -> cand 154769ns)
  • get_field_map_1024_entries_4_last: 11.166% faster (base 23229ns -> cand 20635ns)
  • get_field_map_1024_entries_4_first: 15.396% faster (base 20877ns -> cand 17663ns)

Full criterion output:

get_field_map_1024_entries_4_first
                        time:   [17.664 µs 17.701 µs 17.756 µs]
                        change: [−15.807% −15.396% −14.964%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  4 (4.00%) high mild
  2 (2.00%) high severe

get_field_map_1024_entries_4_last
                        time:   [20.450 µs 20.499 µs 20.546 µs]
                        change: [−11.535% −11.166% −10.793%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  2 (2.00%) low mild
  4 (4.00%) high mild

get_field_map_1024_entries_16_last
                        time:   [39.406 µs 39.460 µs 39.516 µs]
                        change: [−6.9089% −6.2297% −5.5570%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 11 outliers among 100 measurements (11.00%)
  6 (6.00%) low severe
  2 (2.00%) high mild
  3 (3.00%) high severe

get_field_map_1024_entries_4_missing
                        time:   [16.755 µs 16.775 µs 16.796 µs]
                        change: [−12.675% −12.267% −11.888%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
  4 (4.00%) high mild
  2 (2.00%) high severe

get_field_map_8192_entries_4_last
                        time:   [154.23 µs 154.92 µs 155.72 µs]
                        change: [−8.5962% −8.1217% −7.6240%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 12 outliers among 100 measurements (12.00%)
  11 (11.00%) high mild
  1 (1.00%) high severe

Are there any user-facing changes?

No

@github-actions github-actions Bot added the functions Changes to functions implementation label Jul 13, 2026
@andygrove andygrove changed the title perf: optimize get_field in datafusion-functions perf: optimize get_field Jul 13, 2026
@andygrove
andygrove marked this pull request as ready for review July 13, 2026 23:50
.enumerate()
.find(|(_, t)| t.unwrap());
let matched = (start..end).find(|&i| {
matches.value(i) && match_nulls.is_none_or(|nulls| nulls.is_valid(i))

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.

the previous logic always unwrapped, meaning it ignored any nulls; i assume this is to do with how map array keys cannot be null. perhaps we can follow on that assumption and ignore null check as before?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good call — map keys are non-null by definition, so the comparison result never carries nulls here. Dropped the check to match the previous behavior (which unwrapped unconditionally). Done in c4ed51d.

@alamb alamb added the performance Make DataFusion faster label Jul 16, 2026
Map keys are non-null by definition, so the comparison result carries no
nulls to check while scanning for a match.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@3a29d6b). Learn more about missing BASE report.

Files with missing lines Patch % Lines
datafusion/functions/src/core/getfield.rs 75.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #23537   +/-   ##
=======================================
  Coverage        ?   80.67%           
=======================================
  Files           ?     1086           
  Lines           ?   366795           
  Branches        ?   366795           
=======================================
  Hits            ?   295902           
  Misses          ?    53262           
  Partials        ?    17631           

☔ 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.

@andygrove
andygrove added this pull request to the merge queue Jul 17, 2026
@andygrove

Copy link
Copy Markdown
Member Author

Thanks for the review @Jefffrey

Merged via the queue into apache:main with commit ca02890 Jul 17, 2026
38 checks passed
@andygrove
andygrove deleted the auto-opt/get_field-datafusion-20260713-142422 branch July 17, 2026 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation performance Make DataFusion faster

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants