Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
156ee11
perf: Optimize `multi_group_by` when there are a lot of unique groups
rluvaton Sep 16, 2025
62ebc86
Merge branch 'main' into optimize-for-unique-groups
rluvaton Sep 16, 2025
3133196
updated based on cr
rluvaton Sep 16, 2025
00b5f07
update comment
rluvaton Sep 16, 2025
9476184
added fuzz test to test all values are unique in aggregate group by
rluvaton Sep 16, 2025
bbc62dc
Merge branch 'main' into optimize-for-unique-groups
rluvaton Sep 16, 2025
a052b39
added comment
rluvaton Sep 16, 2025
390bd68
don't always call `append_array_slice` if it is not supported
rluvaton Sep 17, 2025
0a78ede
add comment
rluvaton Sep 17, 2025
c1bcb17
add benchmark
rluvaton Sep 18, 2025
cc2e725
try optimization
rluvaton Sep 18, 2025
35c5a02
Revert "try optimization"
rluvaton Sep 18, 2025
093b069
reserve and use extend
rluvaton Sep 18, 2025
aa56172
Merge branch 'refs/heads/main' into optimize-for-unique-groups
rluvaton Oct 8, 2025
cbd7fc0
Merge branch 'main' into optimize-for-unique-groups
rluvaton Oct 8, 2025
1ce9617
support `append_array_slice` for boolean group values
rluvaton Oct 8, 2025
045020e
avoid push and use extend instead to make the compiler vectorize the …
rluvaton Oct 8, 2025
e8a94e1
fix: actually generate a lot of unique values in benchmark table
rluvaton Oct 8, 2025
42df966
Merge branch 'main' into fix-benchmark-value-generation
rluvaton Oct 8, 2025
e690921
Merge branch 'fix-benchmark-value-generation' into optimize-for-uniqu…
rluvaton Oct 8, 2025
155868d
format
rluvaton Oct 8, 2025
ff1a744
Merge branch 'fix-benchmark-value-generation' into optimize-for-uniqu…
rluvaton Oct 8, 2025
f4d0373
added multi group by benchmark on primitive only columns
rluvaton Oct 8, 2025
8f2974d
Merge branch 'fix-benchmark-value-generation' into optimize-for-uniqu…
rluvaton Oct 8, 2025
c9cbe59
Merge branch 'main' into optimize-for-unique-groups
rluvaton Nov 20, 2025
7d8db1a
optimize bytes
rluvaton Nov 20, 2025
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
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use arrow::array::{
GenericBinaryArray, GenericByteArray, GenericStringArray, OffsetSizeTrait,
};
use arrow::buffer::{OffsetBuffer, ScalarBuffer};
use arrow::datatypes::{ByteArrayType, DataType, GenericBinaryType};
use arrow::datatypes::{ArrowNativeType, ByteArrayType, DataType, GenericBinaryType};
use datafusion_common::utils::proxy::VecAllocExt;
use datafusion_common::{DataFusionError, Result};
use datafusion_physical_expr_common::binary_map::{OutputType, INITIAL_BUFFER_CAPACITY};
Expand Down Expand Up @@ -171,6 +171,74 @@ where
Ok(())
}

fn append_array_slice_inner<B>(
&mut self,
array: &ArrayRef,
start: usize,
length: usize,
) -> Result<()>
where
B: ByteArrayType,
{
let array = array.as_bytes::<B>();

let offsets = array.offsets();
let bytes = array.value_data();

// Is the slice all nulls
let all_nulls: bool;

// 1. Append the nulls
if let Some(nulls) = array.nulls().filter(|n| n.null_count() > 0) {
let nulls_slice = nulls.slice(start, length);
all_nulls = nulls_slice.null_count() == length;
self.nulls.append_buffer(&nulls_slice);
} else {
all_nulls = false;
self.nulls.append_n(length, false);
}

let values_start = offsets[start].as_usize();
let values_end = offsets[start + length].as_usize();

// 2. Append the offsets

// Must be before adding the bytes to the buffer
let mut last_offset = self.buffer.len();

if all_nulls {
// If all nulls, we can just repeat the last offset
for _ in 0..length {
self.offsets.push(O::usize_as(last_offset));
}
} else {
for start_and_end_values in offsets[start..=start + length].windows(2) {
let length = start_and_end_values[1] - start_and_end_values[0];
last_offset += length.as_usize();
self.offsets.push(O::usize_as(last_offset));
}
}

// 3. Append the bytes

// Only if not all nulls append the actual bytes
if !all_nulls {
// Note: if the array have nulls we might copy some bytes that are not used.

// Add all the bytes for the values directly to the byte buffer
self.buffer.append_slice(&bytes[values_start..values_end]);

if self.buffer.len() > self.max_buffer_size {
return Err(DataFusionError::Execution(format!(
"offset overflow, buffer size > {}",
self.max_buffer_size
)));
}
}

Ok(())
}

fn do_equal_to_inner<B>(
&self,
lhs_row: usize,
Expand Down Expand Up @@ -327,6 +395,37 @@ where
Ok(())
}

fn append_array_slice(
&mut self,
column: &ArrayRef,
start: usize,
length: usize,
) -> Result<()> {
match self.output_type {
OutputType::Binary => {
debug_assert!(matches!(
column.data_type(),
DataType::Binary | DataType::LargeBinary
));
self.append_array_slice_inner::<GenericBinaryType<O>>(
column, start, length,
)?
}
OutputType::Utf8 => {
debug_assert!(matches!(
column.data_type(),
DataType::Utf8 | DataType::LargeUtf8
));
self.append_array_slice_inner::<GenericStringType<O>>(
column, start, length,
)?
}
_ => unreachable!("View types should use `ArrowBytesViewMap`"),
};

Ok(())
}

fn len(&self) -> usize {
self.offsets.len() - 1
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,24 @@ pub trait GroupColumn: Send + Sync {
/// The vectorized version `append_val`
fn vectorized_append(&mut self, array: &ArrayRef, rows: &[usize]) -> Result<()>;

/// Append slice of values from `array`, starting at `start` for `length` rows
///
/// This is a special case of `vectorized_append` when the rows are continuous
///
/// You should implement this to optimize large copies of contiguous values.
///
/// This does not get the sliced array even though it would be more user-friendly
/// to allow optimization that avoid the additional computation that can happen in a slice
fn append_array_slice(
&mut self,
array: &ArrayRef,
start: usize,
length: usize,
) -> Result<()> {
let rows = (start..start + length).collect::<Vec<_>>();
self.vectorized_append(array, &rows)
}
Comment on lines +91 to +117

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.

Originally I did not had support_append_array_slice and I had this default implementation for append_array_slice:

fn append_array_slice(
        &mut self,
        array: &ArrayRef,
        start: usize,
        length: usize,
    ) -> Result<()> {
        let rows = (start..start + length).collect::<Vec<_>>();
        self.vectorized_append(array, &rows)
    }

but I moved out of this to avoid having the allocation here as we already hold the append_row_indices.
as I saw from the benchmarks that it can be slower for non supported impl


/// Returns the number of rows stored in this builder
fn len(&self) -> usize;

Expand Down Expand Up @@ -233,6 +251,16 @@ struct VectorizedOperationBuffers {
/// The `vectorized append` row indices buffer
append_row_indices: Vec<usize>,

/// The last row index in `append_row_indices`
///
/// Will be None if `append_row_indices` is empty
last_append_row_index: Option<usize>,
Comment thread
rluvaton marked this conversation as resolved.
Outdated

/// If all the values in `append_row_indices` are continuous
/// i.e. `append_row_indices[i] + 1 == append_row_indices[i + 1]`
/// this is used to optimize the `vectorized_append` operation
are_row_indices_continuous: bool,
Comment thread
rluvaton marked this conversation as resolved.
Outdated

/// The `vectorized_equal_to` row indices buffer
equal_to_row_indices: Vec<usize>,

Expand All @@ -250,12 +278,28 @@ struct VectorizedOperationBuffers {

impl VectorizedOperationBuffers {
fn clear(&mut self) {
self.append_row_indices.clear();
self.clear_append_row_indices();
self.equal_to_row_indices.clear();
self.equal_to_group_indices.clear();
self.equal_to_results.clear();
self.remaining_row_indices.clear();
}

fn add_append_row_index(&mut self, row: usize) {

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.

It would probably faster and somewhat cleaner to do this in a single check on append_row_indices in vectorized_append,

something like:

append_row_indices.windows(2).all(|[x, y]| x + 1 == y)

will do.

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 even better, it should also be possible to just check the length of append_row_indices. If this is equal to the size of the incoming batch, all of the indices should be consecutive already (i.e. all values are unique), so it becomes a really cheap check.

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.

I think even better, it should also be possible to just check the length of append_row_indices. If this is equal to the size of the incoming batch, all of the indices should be consecutive already (i.e. all values are unique), so it becomes a really cheap check.

but what if we only need to add the last half of the column. then the length would not be equal but it is still consecutive

self.are_row_indices_continuous = self.are_row_indices_continuous
&& self
.last_append_row_index
.is_none_or(|last| last + 1 == row);
self.last_append_row_index = Some(row);

self.append_row_indices.push(row);
}

fn clear_append_row_indices(&mut self) {
self.append_row_indices.clear();
self.are_row_indices_continuous = true;
self.last_append_row_index = None;
}
}

impl<const STREAMING: bool> GroupValuesColumn<STREAMING> {
Expand Down Expand Up @@ -498,7 +542,7 @@ impl<const STREAMING: bool> GroupValuesColumn<STREAMING> {
batch_hashes: &[u64],
groups: &mut [usize],
) {
self.vectorized_operation_buffers.append_row_indices.clear();
self.vectorized_operation_buffers.clear_append_row_indices();
self.vectorized_operation_buffers
.equal_to_row_indices
.clear();
Expand Down Expand Up @@ -528,9 +572,7 @@ impl<const STREAMING: bool> GroupValuesColumn<STREAMING> {
);

// Add row index to `vectorized_append_row_indices`
self.vectorized_operation_buffers
.append_row_indices
.push(row);
self.vectorized_operation_buffers.add_append_row_index(row);

// Set group index to row in `groups`
groups[row] = current_group_idx;
Expand Down Expand Up @@ -578,11 +620,24 @@ impl<const STREAMING: bool> GroupValuesColumn<STREAMING> {
}

let iter = self.group_values.iter_mut().zip(cols.iter());
for (group_column, col) in iter {
group_column.vectorized_append(
col,
&self.vectorized_operation_buffers.append_row_indices,
)?;
if self.vectorized_operation_buffers.are_row_indices_continuous
&& !self
.vectorized_operation_buffers
.append_row_indices
.is_empty()
{
let start = self.vectorized_operation_buffers.append_row_indices[0];
let length = self.vectorized_operation_buffers.append_row_indices.len();
for (group_column, col) in iter {
group_column.append_array_slice(col, start, length)?;
}
} else {
for (group_column, col) in iter {
group_column.vectorized_append(
col,
&self.vectorized_operation_buffers.append_row_indices,
)?;
}
}

Ok(())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,35 @@ impl<T: ArrowPrimitiveType, const NULLABLE: bool> GroupColumn
Ok(())
}

fn append_array_slice(
&mut self,
array: &ArrayRef,
start: usize,
length: usize,
) -> Result<()> {
let array = array.as_primitive::<T>();

if NULLABLE {
if let Some(nulls) = array.nulls().filter(|n| n.null_count() > 0) {
self.nulls.append_buffer(&nulls.slice(start, length));
} else {
self.nulls.append_n(length, false);
}
} else {
assert_eq!(
array.null_count(),
0,
"unexpected nulls in non nullable input"
);
self.nulls.append_n(length, false);
}

self.group_values
.extend_from_slice(&array.values()[start..start + length]);

Ok(())
}

fn len(&self) -> usize {
self.group_values.len()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,13 @@ impl MaybeNullBufferBuilder {
}
}

/// Append [`NullBuffer`] to this [`NullBufferBuilder`]
///
/// This is useful when you want to concatenate two null buffers.
pub fn append_buffer(&mut self, other: &NullBuffer) {
self.nulls.append_buffer(other);
}

/// return the number of heap allocated bytes used by this structure to store boolean values
pub fn allocated_size(&self) -> usize {
// NullBufferBuilder builder::allocated_size returns capacity in bits
Expand Down