jayzhan211 commented on PR #24714:
URL: https://github.com/apache/datafusion/pull/24714#issuecomment-5462342705
**`values_preserving` fails for every non-nullable primitive/boolean group
column.**
`NullBufferBuilderExt::build_preserving` validates the selection against the
null builder's length:
```rust
fn build_preserving(&self, selection: GroupSelection<'_>) ->
Result<Option<NullBuffer>> {
selection.validate_num_groups(self.len())?;
```
For `PrimitiveGroupValueBuilder<T, false>` and
`BooleanGroupValueBuilder<false>`, `append_val` / `vectorized_append` never
touch `self.nulls`, so `NullBufferBuilder::len()` is 0 while the column holds N
rows. Every preserving read on a `NOT NULL` grouping column therefore errors.
`instantiate_primitive!` selects `NULLABLE` from `field.is_nullable()`, so this
is the common `GROUP BY not_null_col` path, not an edge case.
Reproduced through the public `GroupValues` entry point:
```rust
let schema = Arc::new(Schema::new(vec![
Field::new("g", DataType::Int32, false),
Field::new("b", DataType::Boolean, false),
]));
let mut gv = new_group_values(schema, &GroupOrdering::None).unwrap();
gv.intern(&[int32_col, bool_col], &mut groups).unwrap();
gv.values_preserving(GroupSelection::all(gv.len()))
// Err(Execution("Group selection was constructed for 2 groups but applied
to 0 groups"))
```
`take_n` already handles this correctly (`if NULLABLE { self.nulls.take_n(n)
} else { None }`); `build()` only escapes it because
`NullBufferBuilder::build()` on a zero-length builder happens to return `None`.
Gate the two callers the same way:
```diff
// multi_group_by/primitive.rs
fn values_preserving(&self, selection: GroupSelection<'_>) ->
Result<ArrayRef> {
selection.validate_num_groups(self.group_values.len())?;
let values: Vec<T::Native> = selection
.iter()
.map(|index| self.group_values[index])
.collect();
- let nulls = self.nulls.build_preserving(selection)?;
+ let nulls = if NULLABLE {
+ self.nulls.build_preserving(selection)?
+ } else {
+ None
+ };
Ok(Arc::new(
PrimitiveArray::<T>::new(ScalarBuffer::from(values), nulls)
.with_data_type(self.data_type.clone()),
))
}
```
```diff
// multi_group_by/boolean.rs
fn values_preserving(&self, selection: GroupSelection<'_>) ->
Result<ArrayRef> {
selection.validate_num_groups(self.buffer.len())?;
let mut values = BooleanBufferBuilder::new(selection.len());
for index in selection.iter() {
values.append(self.buffer.get_bit(index));
}
- let nulls = self.nulls.build_preserving(selection)?;
+ let nulls = if NULLABLE {
+ self.nulls.build_preserving(selection)?
+ } else {
+ None
+ };
Ok(Arc::new(BooleanArray::new(values.finish(), nulls)))
}
```
Every new test in this PR uses nullable fields, which is why CI is green.
Please extend `test_preserving_selected_vectorized_group_values` (or add a case
in `group_values/mod.rs`) to cover `nullable = false` so this stays fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
