Skip to content

chore: use new OffsetBuffer::subtract helper #23424

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
rluvaton wants to merge 3 commits into
base: main
Choose a base branch
from
+12 −38
Open

chore: use new OffsetBuffer::subtract helper #23424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a0b3aaf
chore: use new `OffsetBuffer::subtract` helper
rluvaton Jul 9, 2026
314fdc3
chore: use new `OffsetBuffer::subtract` helper
rluvaton Jul 9, 2026
51365ae
update comment
rluvaton Jul 9, 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
11 changes: 1 addition & 10 deletions datafusion/common/src/utils/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1236,16 +1236,7 @@ pub fn adjust_offsets_for_slice<O: OffsetSizeTrait>(
) -> OffsetBuffer<O> {
let offsets = list.offsets();

if let (Some(first), Some(last)) = (offsets.first(), offsets.last())
&& (!first.is_zero() || last.as_usize() != list.values().len())
{
let offsets = offsets.iter().map(|offset| *offset - *first).collect();

//todo: use unsafe Offset::new_unchecked?
return OffsetBuffer::new(offsets);
}

offsets.clone()
Comment on lines -1239 to -1248

@rluvaton rluvaton Jul 9, 2026 •
edited
Loading

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.

Even though this code has last != list.values().len() and the subtract doesn't have that check, that check is not needed as we don't use the last values at all

offsets.clone().subtract(offsets[0])

@xudong963 xudong963 Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For a prefix slice where the first offset is zero but the last visible offset is before the end of the original child values, subtract(0) returns a clone sharing the original allocation. The previous
Implementation created a compact offset buffer in this case.

@rluvaton rluvaton Jul 10, 2026

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.

Yes, I'm willing to sacrifice that in favor of avoiding copy and because this was not guaranteed

}

/// For lists and large lists, truncates the sublist of null values
Expand Down
7 changes: 1 addition & 6 deletions datafusion/functions-nested/src/sort.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -471,12 +471,7 @@ fn take_by_indices<OffsetSize: OffsetSizeTrait>(
fn rebase_offsets<OffsetSize: OffsetSizeTrait>(
offsets: &OffsetBuffer<OffsetSize>,
) -> OffsetBuffer<OffsetSize> {
if offsets[0].as_usize() == 0 {
offsets.clone()
} else {
let rebased: Vec<OffsetSize> = offsets.iter().map(|o| *o - offsets[0]).collect();
OffsetBuffer::new(rebased.into())
}
offsets.clone().subtract(offsets[0])
}

fn order_desc(modifier: &str) -> Result<bool> {
Expand Down
15 changes: 5 additions & 10 deletions datafusion/functions/src/string/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use arrow::array::{
Array, ArrayRef, AsArray, GenericStringArray, NullBufferBuilder, OffsetSizeTrait,
StringViewArray, new_null_array,
};
use arrow::buffer::{Buffer, OffsetBuffer, ScalarBuffer};
use arrow::buffer::{Buffer, ScalarBuffer};
use arrow::datatypes::DataType;
use datafusion_common::Result;
use datafusion_common::cast::{as_generic_string_array, as_string_view_array};
Expand Down Expand Up @@ -636,15 +636,10 @@ fn case_conversion_ascii_array<O: OffsetSizeTrait>(
let values = Buffer::from_vec(converted);

// Shift offsets from `start`-based to 0-based so they index into `values`.
let offsets = if start == 0 {
string_array.offsets().clone()
} else {
let s = O::usize_as(start);
let rebased: Vec<O> = value_offsets.iter().map(|&o| o - s).collect();
// SAFETY: subtracting a constant from monotonic offsets preserves
// monotonicity, and `start` is the minimum offset, so no underflow.
unsafe { OffsetBuffer::new_unchecked(ScalarBuffer::from(rebased)) }
};
let offsets = string_array
.offsets()
.clone()
.subtract(string_array.offsets()[0]);

let nulls = string_array.nulls().cloned();
// SAFETY: offsets are monotonic and in-bounds for `values`; nulls
Expand Down
17 changes: 5 additions & 12 deletions datafusion/functions/src/unicode/initcap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
use std::sync::Arc;

use arrow::array::{Array, ArrayRef, GenericStringArray, OffsetSizeTrait};
use arrow::buffer::{Buffer, OffsetBuffer};
use arrow::buffer::Buffer;
use arrow::datatypes::DataType;

use crate::strings::{GenericStringArrayBuilder, StringViewArrayBuilder};
Expand Down Expand Up @@ -217,17 +217,10 @@ fn initcap_ascii_array<T: OffsetSizeTrait>(
}

let values = Buffer::from_vec(out);
let out_offsets = if first_offset == 0 {
offsets.clone()
} else {
// For sliced arrays, we need to rebase the offsets to reflect that the
// output only contains the bytes in the visible slice.
let rebased_offsets = offsets
.iter()
.map(|offset| T::usize_as(offset.as_usize() - first_offset))
.collect::<Vec<_>>();
OffsetBuffer::<T>::new(rebased_offsets.into())
};

// Rebase offsets for sliced arrays to reflect that the
// output only contains the bytes in the visible slice.
let out_offsets = offsets.clone().subtract(offsets[0]);

// SAFETY: ASCII case conversion preserves byte length, so the original
// string boundaries are preserved. `out_offsets` is either identical to
Expand Down
Loading