Skip to content

Optimize arrow bytes view map #20055

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.

Sign up for GitHub

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

Open
Tushar7012 wants to merge 16 commits into
base: main
Choose a base branch
from
+5 −4
Open

Optimize arrow bytes view map #20055

Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
51768bf
perf: Optimize ArrowBytesViewMap with direct view access
Tushar7012 Jan 24, 2026
cff1f46
Merge branch 'main' into optimize-arrow-bytes-view-map
Tushar7012 Jan 25, 2026
e3cd6d4
reduce Entry memory by using u32 for builder index
Tushar7012 Jan 25, 2026
b9f7d68
perf: store views directly, remove builder_idx from Entry
Tushar7012 Jan 25, 2026
52b6f78
use BooleanBufferBuilder for null tracking
Tushar7012 Jan 26, 2026
32bd8b1
Merge branch 'main' into optimize-arrow-bytes-view-map
Tushar7012 Jan 26, 2026
49de0e9
Apply review suggestions: remove unnecessary & 0xFFFFFFFF masks
Tushar7012 Jan 26, 2026
d2929e1
Merge branch 'main' into optimize-arrow-bytes-view-map
Tushar7012 Jan 26, 2026
5996423
fix: apply cargo fmt formatting
Tushar7012 Jan 26, 2026
9c06c58
fix: use std::mem::take per clippy suggestion
Tushar7012 Jan 26, 2026
9302c3b
revert: remove accidental table.rs changes (belongs in separate PR)
Tushar7012 Jan 26, 2026
c9be2af
Merge branch 'main' into optimize-arrow-bytes-view-map
Tushar7012 Jan 26, 2026
0fc43a4
perf: Optimize ArrowBytesViewMap with direct view access and single f…
Tushar7012 Jan 28, 2026
6a73668
merge: resolve conflicts in binary_view_map.rs by prioritizing single…
Tushar7012 Jan 28, 2026
102855a
perf: defer byte slice fetch in ArrowBytesViewMap to avoid unnecessar…
Tushar7012 Jan 29, 2026
f105b53
chore: apply cargo fmt to binary_view_map.rs
Tushar7012 Jan 29, 2026
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
9 changes: 5 additions & 4 deletions datafusion/physical-expr-common/src/binary_view_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,8 @@ where

// Extract length from the view (first 4 bytes of u128 in little-endian)
let len = view_u128 as u32;

// Check if value already exists
let mut value: Option<&[u8]> = None;
Copy link
Contributor

@Dandandan Dandandan Jan 29, 2026

Choose a reason for hiding this comment

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

This seems not right?

Copy link
Contributor Author

@Tushar7012 Tushar7012 Jan 29, 2026

Choose a reason for hiding this comment

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

Good catch — you’re right to flag this.

The intent here was to defer materializing the byte slice until we know it’s needed, but the current structure makes that unclear and introduces unnecessary indirection.

I’ll refactor this so the slice is explicitly materialized once (at the top of the loop) and then reused consistently for both lookup and insertion paths. That should make the logic clearer and align better with the original optimization goal.

let maybe_payload = {
// Borrow completed and in_progress for comparison
let completed = &self.completed;
Expand Down Expand Up @@ -328,8 +328,9 @@ where
} else {
&in_progress[offset..offset + stored_len]
Comment on lines 328 to 329
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

The condition checking which buffer to read from should be more defensive. If buffer_index > completed.len(), this would incorrectly try to access in_progress with potentially invalid offsets. While this shouldn't happen with the current append logic, consider adding an assertion or explicit error handling to catch any future bugs. For example: else if buffer_index == completed.len() followed by else { panic!("Invalid buffer_index") }.

Suggested change
} else {
&in_progress[offset..offset + stored_len]
} else if buffer_index == completed.len() {
&in_progress[offset..offset + stored_len]
} else {
panic!("Invalid buffer_index: {}", buffer_index);

Copilot uses AI. Check for mistakes.
};
let input_value: &[u8] = values.value(i).as_ref();
stored_value == input_value
let input_value =
value.get_or_insert_with(|| values.value(i).as_ref());
stored_value == *input_value
})
.map(|entry| entry.payload)
};
Expand All @@ -338,7 +339,7 @@ where
payload
} else {
// no existing value, make a new one
let value: &[u8] = values.value(i).as_ref();
let value = value.unwrap_or_else(|| values.value(i).as_ref());
Copy link
Contributor

@Dandandan Dandandan Jan 29, 2026

Choose a reason for hiding this comment

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

This still accesses it here?

Copy link
Contributor Author

@Tushar7012 Tushar7012 Jan 29, 2026

Choose a reason for hiding this comment

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

Yes, you’re absolutely right — this still ends up calling values.value(i) in the insert path.

That contradicts the goal of fetching the value once per row. I’ll rework this to eagerly bind the slice to a local variable and thread that through both the lookup and insert branches so values.value(i) is only accessed a single time.

let payload = make_payload_fn(Some(value));

// Create view pointing to our buffers
Expand Down