Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
398 changes: 398 additions & 0 deletions datafusion/expr-common/src/blocked_groups_accumulator.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,398 @@
// Licensed to the Apache Software Foundation (ASF) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The ASF licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.

//! Vectorized [`GroupsAccumulator`]

use crate::groups_accumulator::EmitTo;
use arrow::array::{ArrayRef, BooleanArray};
use datafusion_common::{
Result, assert_ne_or_internal_err, assert_or_internal_err, exec_err, not_impl_err,
};
use std::cmp::Ordering;

/// Selects groups for a non-destructive grouped aggregation read.
///
/// Unlike [`BlockedEmitTo`], this selection does not remove groups or change their
/// indices. Selections created by [`Self::try_from_indices`] preserve the
/// requested order and support duplicate indices.
///
/// A selection is validated once when it is constructed and can then be reused
/// for the group values and accumulators participating in the same snapshot.
/// Construct a new selection if the number or indexing of those groups changes.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub struct BlockedGroupSelection<'a> {
block_size: usize,
total_num_groups: usize,
indices: Option<&'a [BlocksIndex]>,
}

impl<'a> BlockedGroupSelection<'a> {
/// Selects all `total_num_groups` groups in group-index order.
pub fn all(total_num_groups: usize, block_size: usize) -> Self {
Self {
total_num_groups,
indices: None,
block_size,
}
}

/// Selects groups in the order specified by `indices`.
///
/// Returns an error if an index is not less than `total_num_groups`. Empty
/// selections are valid, and duplicate indices are preserved.
pub fn try_from_indices(
indices: &'a [BlocksIndex],
total_num_groups: usize,
block_size: usize,
) -> Result<Self> {
let total_num_groups_parsed =
BlocksIndex::from_index_in_fixed_block_size(total_num_groups, block_size);
if let Some(index) = indices
.iter()
.find(|&&index| index >= total_num_groups_parsed)
{
return exec_err!(
"Group index {index:?} is out of bounds for {total_num_groups_parsed:?} ({total_num_groups}) groups"
);
}
Ok(Self {
total_num_groups,
indices: Some(indices),
block_size,
})
}

/// Returns the group count against which this selection was constructed.
pub fn total_num_groups(self) -> usize {
self.total_num_groups
}

/// Ensures this selection is being applied to the same number of groups
/// against which it was constructed.
///
/// Preserving-read implementations should call this method with their
/// stored group count before using [`Self::iter`]. This check is `O(1)`;
/// the selected indices were already checked by [`Self::try_from_indices`].
pub fn validate_num_groups(self, actual_num_groups: usize) -> Result<()> {
if actual_num_groups != self.total_num_groups {
return exec_err!(
"Group selection was constructed for {} groups but applied to {actual_num_groups} groups",
self.total_num_groups
);
}
Ok(())
}

/// Returns the number of selected groups.
pub fn len(self) -> usize {
self.indices
.map_or(self.total_num_groups, |indices| indices.len())
}

/// Returns `true` if no groups are selected.
pub fn is_empty(self) -> bool {
self.len() == 0
}

/// Returns the selected group indices in output order.
pub fn iter(self) -> impl Iterator<Item = BlocksIndex> + 'a {
let (all, indices): (_, &'a [BlocksIndex]) = match self.indices {
None => (0..self.total_num_groups, &[]),
Some(indices) => (0..0, indices),
};
let block_size = self.block_size;
all.map(move |index| {
BlocksIndex::from_index_in_fixed_block_size(index, block_size)
})
.chain(indices.iter().copied())
}

#[doc(hidden)]
pub fn indices(&self) -> Option<&[BlocksIndex]> {
self.indices
}
}

/// Two `u32`s (8 bytes) rather than two `usize`s: an index is stored per group in every blocked
/// hash table entry, so its size decides the size of the tables. Both halves fit `u32` by a wide
/// margin, `index_in_block < block_size` (the batch size) and the number of blocks is
/// `groups / block_size`. The API keeps taking and returning `usize`.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Hash)]
pub struct BlocksIndex {

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.

BlocksIndex puts a lot of public surface in expr-common: sub_flat, sub_flat_checked (L199), gte_flat, prev_block, add, increment. Only group-values implementations need these. Suggest keeping the public type to new, block_index, index_in_block and the flat conversions, and moving the rest next to their only user, or marking them #[doc(hidden)].

block_index: u32,
index_in_block: u32,
}

impl BlocksIndex {
pub const ZERO: Self = Self {
// flat_index: 0,
block_index: 0,
index_in_block: 0,
};
pub const MAX: Self = Self {
// flat_index: usize::MAX,
block_index: u32::MAX,
index_in_block: u32::MAX,
};

pub fn new(block_index: usize, index_in_block: usize) -> Self {
Self {
block_index: Self::narrow(block_index),
index_in_block: Self::narrow(index_in_block),
}
}

#[inline(always)]
fn narrow(value: usize) -> u32 {
debug_assert!(
u32::try_from(value).is_ok(),
"block index part {value} does not fit in u32"
);
value as u32
}

#[inline(always)]
pub fn new_in_first_block(index_in_block: usize) -> Self {
// Implementation note:
// not having From<usize> that will do this instead even when it will be more convenient
// so we can later change the layout to be a single usize with bit shifts
Self::new(0, index_in_block)
// Self {
// flat_index: index_in_block,
// }
}

#[inline(always)]
pub fn from_index_in_fixed_block_size(index: usize, block_size: usize) -> Self {
Self::new(index / block_size, index % block_size)
}

#[inline(always)]
pub fn into_index_in_fixed_block_size(self, block_size: usize) -> usize {
self.block_index as usize * block_size + self.index_in_block as usize
}

#[inline(always)]
pub fn block_index(&self) -> usize {
self.block_index as usize
}

#[inline(always)]
pub fn index_in_block(&self) -> usize {
self.index_in_block as usize
}

pub fn sub_flat_checked(self, rhs_flat: usize, block_size: usize) -> Option<Self> {
let self_flat = self.into_index_in_fixed_block_size(block_size);
self_flat
.checked_sub(rhs_flat)
.map(|v| Self::from_index_in_fixed_block_size(v, block_size))
}

pub fn sub_flat(self, rhs_flat: usize, block_size: usize) -> Self {
Self::from_index_in_fixed_block_size(
self.into_index_in_fixed_block_size(block_size) - rhs_flat,
block_size,
)
}

pub fn gte_flat(self, rhs_flat: usize, block_size: usize) -> bool {
self.into_index_in_fixed_block_size(block_size) >= rhs_flat
}

pub fn prev_block(mut self) -> Self {
self.block_index -= 1;

self
}

pub fn prev_block_checked(mut self) -> Option<Self> {
self.block_index.checked_sub(1).map(|v| {
self.block_index = v;
self
})
}

pub fn add(self, n: usize, block_size: usize) -> BlocksIndex {
if self.index_in_block + n as u32 >= block_size as u32 {
let new_flat = self.into_index_in_fixed_block_size(block_size) + n;
Self::from_index_in_fixed_block_size(new_flat, block_size)
} else {
Self::new(self.block_index as usize, self.index_in_block as usize + n)
}
}

pub fn increment(mut self, block_size: usize) -> Self {
if self.index_in_block + 1 >= block_size as u32 {
self.block_index += 1;
self.index_in_block = 0;
} else {
self.index_in_block += 1;
}

self
}
}

impl PartialOrd for BlocksIndex {
fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
Some(self.cmp(other))
}
}

impl Ord for BlocksIndex {
fn cmp(&self, other: &Self) -> Ordering {
self.block_index
.cmp(&other.block_index)
.then(self.index_in_block.cmp(&other.index_in_block))
}
}

/// Describes how many rows should be emitted during grouping.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum BlockedEmitTo {
All,
/// Emit next group
NextBlock,
/// Emit only the first `n` groups and shift all existing group
/// indexes down by `n`.
///
/// For example, if `n=10`, group_index `0, 1, ... 9` are emitted
/// and group indexes `10, 11, 12, ...` become `0, 1, 2, ...`.
///
/// Requirements:
/// 1. `n` must be smaller than block_size
/// 2. `n` is not 0
First(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.

The First(n) precondition n < block_size leaks to callers: common_ordered.rs:481 already rebuilds EmitTo::First(n) as NextBlocks plus a First remainder. Either allow any n, or define that decomposition once, next to the enum:

impl BlockedEmitTo {
    /// Splits a flat `EmitTo` into block-sized emits.
    pub fn from_emit_to(emit_to: EmitTo, block_size: usize) -> Vec<Self> {
        match emit_to {
            EmitTo::All => vec![Self::All],
            EmitTo::First(n) => {
                let mut emits = vec![Self::NextBlock; n / block_size];
                if n % block_size != 0 {
                    emits.push(Self::First(n % block_size));
                }
                emits
            }
        }
    }
}

}

impl BlockedEmitTo {
pub fn into_emit_to(
self,
len: usize,
block_size: usize,
) -> Result<impl Iterator<Item = EmitTo> + Clone> {
let mut to_emit = match self {
Self::All => len,
Self::NextBlock => len.min(block_size),
Self::First(n) => {
assert_ne_or_internal_err!(n, 0);
assert_or_internal_err!(
n <= len,
"n ({n}) must be less than or equal current length ({})",
len
);
assert_or_internal_err!(
n < block_size,
"n ({n}) must be less than current block size ({})",
block_size
);
n
}
};

let mut blocks = vec![];

let emitting_all = len == to_emit;

while to_emit > block_size {
blocks.push(EmitTo::First(block_size));
to_emit -= block_size
}

if to_emit > 0 && emitting_all {
blocks.push(EmitTo::All);
} else if to_emit > 0 {
blocks.push(EmitTo::First(to_emit));
}

Ok(blocks.into_iter())
}
}

pub trait BlockedGroupsAccumulator: Send + std::any::Any {

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.

This trait repeats GroupsAccumulator method for method, and BlockedGroupSelection (L37) repeats GroupSelection; only the index type changes. Could the two share a generic index type, or could the blocked methods be defaulted on GroupsAccumulator? That would avoid two parallel public traits that have to stay in sync.

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.

BlockedGroupsAccumulator would have to extend GroupsAccumulator and I don't want that since every function call now will be confusion to what is being called and also the plan is to remove GroupsAccumulator from the way I see it and not keep it, so adding dependencies to old implementation would just couple more the implementations

fn batch_size(&self) -> 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.

nit: this is named batch_size(), but everything else (BlockedVec::block_size, BlockedGroupValues::block_size) says block size. Suggest fn block_size(&self) -> usize;, and also renaming BlockedAccumulatorArgs::batch_size.


fn update_batch(
&mut self,
values: &[ArrayRef],
// Get blocks index rather than flat index to avoid extra computation when inserting and searching (BlocksIndex can be implemented as flat usize)
group_indices: &[BlocksIndex],
opt_filter: Option<&BooleanArray>,
total_num_groups: usize,
) -> Result<()>;

/// Same as `GroupsAccumulator::evaluate` but returning vec of blocks and different EmitTo
/// For
/// - `BlockedEmitTo::All` it should return `Vec<Block>`
/// - `BlockedEmitTo::NextBlock` it should return single item vector with the block or empty vec in case of no blocks
/// - `BlockedEmitTo::First(n)` it should return single item vector with the first n rows in the first block. n must be smaller than block size and length
///
fn evaluate(&mut self, emit_to: BlockedEmitTo) -> Result<Vec<ArrayRef>>;

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.

Return shapes:

  • NextBlock yields 0 or 1 blocks, so returning a Vec hides that; Option<ArrayRef> (or a dedicated evaluate_next_block) would say so.
  • evaluate_preserving (L348) and state_preserving return one flat array rather than blocks, which is inconsistent with evaluate and state.
  • If the plan is an iterator for emit All (the TODO in group_values/blocked.rs), it's cheaper to put it in the trait now than to change it later.

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.

evaluate_preserving is like that since it provide the indices, I can split the indices by block size and return Vec

Return shapes:

  • NextBlock yields 0 or 1 blocks, so returning a Vec hides that; Option<ArrayRef> (or a dedicated evaluate_next_block) would say so.
    I thought about it originally but in order to not explode the code with functions to implement I thought this will be a better way, what do you think?
  • evaluate_preserving (L348) and state_preserving return one flat array rather than blocks, which is inconsistent with evaluate and state.

evaluate_preserving/state_preserving is like that since it provide the indices, I can split the indices by block size and return Vec if that is what you mean

  • If the plan is an iterator for emit All (the TODO in group_values/blocked.rs), it's cheaper to put it in the trait now than to change it later.

the iterator that emit all is just an idea, this will be useful when materializing will use more memory than needed

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.

having iterator create challenges regarding memory size


// Same as `GroupsAccumulator::evaluate_preserving` but with blocked indices
fn evaluate_preserving(
&mut self,
_selection: BlockedGroupSelection<'_>,
) -> Result<ArrayRef> {
not_impl_err!("Preserving grouped evaluation is not implemented")
}

/// Returns `true` if [`Self::evaluate_preserving`] is implemented.
fn supports_evaluate_preserving(&self) -> bool {
false
}

/// Same as `GroupsAccumulator::state` but returning vec of blocks and different EmitTo
/// For
/// - `BlockedEmitTo::All` it should return `Vec<Block>`
/// - `BlockedEmitTo::NextBlock` it should return single item vector with the block or empty vec in case of no blocks
/// - `BlockedEmitTo::First(n)` it should return single item vector with the first n rows in the first block. n must be smaller than block size and length
///
fn state(&mut self, emit_to: BlockedEmitTo) -> Result<Vec<Vec<ArrayRef>>>;

/// Same as `GroupsAccumulator::state_preserving` but with blocked indices
fn state_preserving(
&mut self,
_selection: BlockedGroupSelection<'_>,
) -> Result<Vec<ArrayRef>> {
not_impl_err!("Preserving grouped state is not implemented")
}

/// Returns `true` if [`Self::state_preserving`] is implemented.
fn supports_state_preserving(&self) -> bool {
false
}
fn merge_batch(
&mut self,
values: &[ArrayRef],

// Get blocks index rather than flat index to avoid extra computation when inserting and searching (BlocksIndex can be implemented as flat usize)
group_indices: &[BlocksIndex],
total_num_groups: usize,
) -> Result<()>;

/// Same as `GroupsAccumulator::convert_to_state`
fn convert_to_state(
&self,
values: &[ArrayRef],
opt_filter: Option<&BooleanArray>,
) -> Result<Vec<ArrayRef>>;

/// Same as `GroupsAccumulator::size`
fn size(&self) -> usize;
}
Loading
Loading