Repository navigation
feat: support preserving reads for state-heavy accumulators - #77
Conversation
| for index in selection.iter() { | ||
| self.groups[index].serialize(&mut scratch); | ||
| builder.append_value(&scratch); |
There was a problem hiding this comment.
Should we check the accumulated bytes to avoid overflow? In other words, return an error rather than panic.
| let array = if selection.is_empty() { | ||
| self.default_orderings[column_index].to_array_of_size(0)? | ||
| } else { | ||
| ScalarValue::iter_to_array(selection.iter().map(|group_index| { |
There was a problem hiding this comment.
Unlike HLL, this path uses ScalarValue::iter_to_array, which also backs destructive state() and supports nested values. A local string/binary check would be incomplete. I think it's worth punting on this for now
| } | ||
| } | ||
|
|
||
| fn build_preserving(&self, selection: GroupSelection<'_>) -> Result<ArrayRef> { |
There was a problem hiding this comment.
nit: the first_last preserving-read test only exercises PrimitiveValueState (Int64), so this BytesValueState::build_preserving (and GenericValueState::build_preserving below) are uncovered — including the unsafe from_utf8_unchecked and the i32/i64 offset-capacity math here. Worth adding a Utf8/string first/last preserving test (and ideally a List/nested one) to lock these paths.
|
|
||
| fn build_state_preserving(&self, selection: GroupSelection<'_>) -> Result<ArrayRef> { | ||
| selection.validate_num_groups(self.group_values.len())?; | ||
| let mut offsets = Vec::with_capacity(selection.len() + 1); |
There was a problem hiding this comment.
nit (optional): this offset-building pattern (offsets vec seeded with 0 + checked_add running total + i32::try_from capacity guard) repeats here, in array_agg.rs build_selected_groups, and in first_last/state.rs BytesValueState::build_preserving. Could factor into a small shared helper that returns the OffsetBuffer; the per-site value flattening differs, so this is low priority — feel free to leave it.
There was a problem hiding this comment.
Thanks for suggestion. Going to leave this unfactored as the sites differ in offset semantics and how the values are materialized
3 participants
Which issue does this PR close?
Rationale for this change
The preserving grouped-read APIs added in #74 are not implemented by the state-heavy grouped accumulators. A caller taking a selective snapshot therefore cannot use those APIs when a plan contains
approx_distinct,array_agg, ordered first/last, median, orpercentile_cont.What changes are included in this PR?
The existing destructive
evaluateandstatepaths are unchanged. Preserving-only allocations and indexing are kept in the new methods.Are these changes tested?
Yes. Unit tests cover repeated reads, reordered and duplicate selections, empty selections, intermediate-state round trips, and updates after a preserving read. I also ran:
cargo fmt --allcargo clippy --all-targets --all-features -- -D warningsforce_hash_collisionsThe workspace run had one memory-threshold test exceed its limit by 2.4 MB; that test passed when rerun in isolation.
Are there any user-facing changes?
The existing optional preserving-read APIs now report support for these grouped accumulators. There are no public API signature changes and no SQL behavior changes.