kumarUjjawal commented on code in PR #24606:
URL: https://github.com/apache/datafusion/pull/24606#discussion_r3843967010
##########
datafusion/physical-plan/src/aggregates/group_values/null_builder.rs:
##########
@@ -93,23 +65,7 @@ impl MaybeNullBufferBuilder {
///
/// This is guaranteed to be true if there are nulls
/// but may be true even if there are no nulls
- pub(crate) fn might_have_nulls(&self) -> bool {
- self.nulls.as_slice().is_some()
- }
-}
-
-#[cfg(test)]
Review Comment:
Would it be worth keeping a trimmed version asserting it directly against
`NullBufferBuilder`?
##########
datafusion/physical-plan/src/aggregates/group_values/null_builder.rs:
##########
@@ -18,71 +18,43 @@
use arrow::array::NullBufferBuilder;
use arrow::buffer::NullBuffer;
-/// Builder for an (optional) null mask
-///
-/// Optimized for avoid creating the bitmask when all values are non-null
-#[derive(Debug)]
-pub(crate) struct MaybeNullBufferBuilder {
- /// Note this is an Arrow *VALIDITY* buffer (so it is false for nulls, true
- /// for non-nulls)
- nulls: NullBufferBuilder,
-}
-
-impl MaybeNullBufferBuilder {
- /// Create a new builder
- pub fn new() -> Self {
- Self {
- nulls: NullBufferBuilder::new(0),
- }
- }
+/// Helper methods for NullBufferBuilder that are used in Group By columns
+pub(crate) trait MaybeNullBufferBuilder {
Review Comment:
should we rename this to `NullBufferBuilderExt` since this is an extension
trait?
##########
datafusion/physical-plan/src/aggregates/group_values/null_builder.rs:
##########
@@ -18,71 +18,43 @@
use arrow::array::NullBufferBuilder;
use arrow::buffer::NullBuffer;
-/// Builder for an (optional) null mask
-///
-/// Optimized for avoid creating the bitmask when all values are non-null
-#[derive(Debug)]
-pub(crate) struct MaybeNullBufferBuilder {
- /// Note this is an Arrow *VALIDITY* buffer (so it is false for nulls, true
- /// for non-nulls)
- nulls: NullBufferBuilder,
-}
-
-impl MaybeNullBufferBuilder {
- /// Create a new builder
- pub fn new() -> Self {
- Self {
- nulls: NullBufferBuilder::new(0),
- }
- }
+/// Helper methods for NullBufferBuilder that are used in Group By columns
+pub(crate) trait MaybeNullBufferBuilder {
+ fn empty() -> Self;
/// Return true if the row at index `row` is null
- pub fn is_null(&self, row: usize) -> bool {
- match self.nulls.as_slice() {
- // validity mask means a unset bit is NULL
- Some(_) => !self.nulls.is_valid(row),
- None => false,
- }
- }
+ fn is_null(&self, row: usize) -> bool;
- /// Set the nullness of the next row to `is_null`
- ///
- /// If `value` is true, the row is null.
- /// If `value` is false, the row is non null
- pub fn append(&mut self, is_null: bool) {
- self.nulls.append(!is_null)
- }
+ /// Returns a NullBuffer representing the first `n` rows accumulated so far
+ /// shifting any remaining down by `n`
+ fn take_n(&mut self, n: usize) -> Option<NullBuffer>;
- pub fn append_n(&mut self, n: usize, is_null: bool) {
- if is_null {
- self.nulls.append_n_nulls(n);
- } else {
- self.nulls.append_n_non_nulls(n);
- }
- }
+ /// Returns true if this builder might have any nulls
Review Comment:
this comment doc is repeated twice, see
https://github.com/apache/datafusion/pull/24606/changes#diff-cbddf809ac36770f6127fb2767ff7932920683b33d7515da206bd7083838e5aaL95
--
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]
