Skip to content

feat: GroupValues::find to look up groups without inserting - #93

Draft
rkrishn7 wants to merge 3 commits into
branch-55from
group-values-find
Draft

rkrishn7 wants to merge 3 commits into
branch-55from
group-values-find

Conversation

@rkrishn7

Copy link
Copy Markdown

Adds a required GroupValues::find: for each input row, the stored group id or None, without inserting and without changing stored values or ids.

fn find(&mut self, cols: &[ArrayRef], groups: &mut Vec<Option<usize>>) -> Result<()>;

fireflow's rolling aggregation will keep groups in several generations and needs to check older ones without growing them.

  • Implemented on all six interners: primitive, column, bytes, bytes_view, boolean, rows. Each mirrors its intern lookup without the insert path.
  • ArrowBytesMap and ArrowBytesViewMap get a lookup-only find with the same null, hash and equality handling as insert_if_new.
  • Required rather than defaulted. new_group_values picks the implementation, so a custom GroupValues can't be plugged into AggregateExec; the break only affects direct implementors.

Relates to apache#24603.

Tests

  • A shared check per dispatched key type (Int32, Utf8, LargeUtf8, Binary, Utf8View, BinaryView, Boolean with null, (Int64, Utf8) → column, (List<Int32>, Int32) → rows): find matches intern, missing rows are None, and len() is unchanged.
  • cargo test -p datafusion-physical-plan --lib group_values (84 passed), also with --features force_hash_collisions; clippy clean.

Implemented for GroupValuesColumn and GroupValuesPrimitive; other implementations report supports_find() == false.
Implement find for GroupValuesBytes, GroupValuesBytesView, GroupValuesBoolean and GroupValuesRows, with lookup-only find on ArrowBytesMap and ArrowBytesViewMap. Remove supports_find.
Streaming mode stores one map entry per group even when hashes collide, so find must check equality inside the probe rather than stopping at the first entry with a matching hash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant