Skip to content

refactor: use raw view access in do_append_val_inner and consolidate duplicated logic#22907

Open
EeshanBembi wants to merge 1 commit into
apache:mainfrom
EeshanBembi:perf/bytes-view-refactor
Open

refactor: use raw view access in do_append_val_inner and consolidate duplicated logic#22907
EeshanBembi wants to merge 1 commit into
apache:mainfrom
EeshanBembi:perf/bytes-view-refactor

Conversation

@EeshanBembi

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Follow-up to #21794, addressing review feedback from @alamb and @Dandandan.

Rationale for this change

In the review of #21794, several optimizations were suggested:

  • @alamb (comment): "as a possible future optimization, we could use get_unchecked here if it makes any difference" — referring to the slow-path arr.views()[row] access.
  • @alamb (comment): "from here on down I think this is basically the same as append_val_inner -- if there are any differences perhaps we can fold it into append_val_inner and avoid the copy"
  • @Dandandan (comment): "In principle we can make this faster as well - extend + reuse input view (instead of make_view) + avoid array.value(row)"

What changes are included in this PR?

1. Refactored do_append_val_inner to use raw view access

Replaced array.value(row) + make_view() with raw view access via get_unchecked(row):

  • Inline (len <= 12): push the u128 view as-is — no decode/re-encode round-trip
  • Non-inline (len > 12): parse via ByteView::from(view), copy buffer data, reuse source prefix directly (avoids re-reading first 4 bytes)

2. Simplified the vectorized slow path

Replaced the duplicated 28-line loop body in vectorized_append_inner with try_reserve + a loop calling do_append_val_inner, eliminating code duplication.

3. Removed unused make_view import

Safety notes

  • get_unchecked usage: Consistent with do_equal_to_inner (same file) and PrimitiveGroupValueBuilder in primitive.rs, both of which use the same pattern. All callers derive row indices from enumeration over the input array length, guaranteeing validity.
  • Buffer access safety: When data_buffers() is empty, all views must have len <= 12 (Arrow invariant), so the non-inline branch is never entered.

Are these changes tested?

Covered by 6 existing unit tests in the bytes_view module plus 3 integration tests in the multi_group_by module. All 111 tests in the aggregates suite pass.

Are there any user-facing changes?

No. This is an internal refactor with no API changes.

…duplicated logic

Refactor `ByteViewGroupValueBuilder::do_append_val_inner` to read raw
u128 views via `get_unchecked` instead of decoding through
`array.value(row)` and re-encoding via `make_view()`. For inline
strings (len <= 12) the view is pushed as-is; for non-inline strings
the source prefix is reused directly.

This also removes the duplicated slow-path logic in
`vectorized_append_inner`, which now delegates to `do_append_val_inner`
after pre-reserving capacity.

The unused `make_view` import is removed.
@github-actions github-actions Bot added the physical-plan Changes to the physical-plan crate label Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant