rkrishn7 commented on PR #24714:
URL: https://github.com/apache/datafusion/pull/24714#issuecomment-5464360314
> Thanks @rkrishn7 for working on this! There's one issue I think we should
resolve before merging:
>
> **`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.
Ah thanks for catching that @jayzhan211 🙌🏾 !
Fixed and added a test with non-null columns in
[721c5e7](https://github.com/apache/datafusion/pull/24714/commits/721c5e787d531bf5186ba22ec2dc218497839f33).
--
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]
