mbutrovich commented on code in PR #4934:
URL: https://github.com/apache/datafusion-comet/pull/4934#discussion_r3591185784
##########
native/spark-expr/src/array_funcs/array_compact.rs:
##########
@@ -117,6 +117,22 @@ fn compact_list<OffsetSize: OffsetSizeTrait>(
};
let values = list_array.values();
+
+ // logical_nulls() (not nulls()) is used so a NullArray, whose elements are
Review Comment:
The comment justifies `logical_nulls` over `is_null`/`nulls` specifically
because a `NullArray` values child reports `nulls() == None` yet is logically
all-null. `array_compact(array(null, null))` produces a `List<Null>` and is
exactly this shape. None of the three new unit tests exercise it, so the branch
the comment protects has no direct coverage and a future refactor to `nulls()`
would pass the suite while silently breaking. This also interacts with finding
1: it is the case that proves `logical_null_count` must be used rather than
`null_count`.
Suggested change: add a test building a `List<Null>` (via
`ListBuilder::new(NullBuilder::new())` or an explicit `NullArray` values child)
and assert all-null rows compact to empty rows.
##########
native/spark-expr/src/array_funcs/array_compact.rs:
##########
@@ -117,6 +117,22 @@ fn compact_list<OffsetSize: OffsetSizeTrait>(
};
let values = list_array.values();
+
+ // logical_nulls() (not nulls()) is used so a NullArray, whose elements are
+ // all logically null, is correctly reported as fully null.
NullArray::nulls()
+ // returns None, which would make is_null() report false for every element.
+ let value_nulls = values.logical_nulls();
Review Comment:
`native/spark-expr/src/array_funcs/array_compact.rs:124,132`
```rust
let value_nulls = values.logical_nulls();
...
if value_nulls.as_ref().map(|n| n.null_count()).unwrap_or(0) == 0 {
return Ok(Arc::new(list_array.clone()));
}
```
For the common element types (primitive, string, struct, list) this is
already cheap. `Array::logical_nulls()` is `self.nulls().cloned()` (arrow-rs
`arrow-array/src/array/mod.rs:243-245`), and since a `NullBuffer` wraps an
`Arc`-backed buffer that clone is an O(1) refcount bump, not a bitmap copy,
while `NullBuffer::null_count()` is a precomputed O(1) read. The default
`logical_null_count()` (`mod.rs:324-328`) is the same clone, so for these types
switching changes nothing.
The case worth guarding against is a values child whose type overrides
`logical_nulls()` to compute a fresh buffer: `DictionaryArray`
(`dictionary_array.rs:741`), `RunArray` (`run_array.rs:717`), and `NullArray`
(`null_array.rs:117`). Each of these also overrides `logical_null_count()`
(`dictionary_array.rs:762`, `run_array.rs:721`, `null_array.rs:125`) to count
without materializing, so for those types `logical_nulls()` is an O(n)
allocation on exactly the fast path this PR exists to speed up while
`logical_null_count()` is not. (`UnionArray` overrides `logical_nulls()` at
`union_array.rs:794` but has no `logical_null_count()` override, so it
allocates either way and does not benefit.) Using `logical_null_count()` makes
the fast path allocation-free for the types that have the override and is never
worse than the current code.
Suggested change: gate the fast path on the count, and only materialize the
buffer on the slow path where it is actually consumed.
```rust
let values = list_array.values();
// Fast path: array_compact only removes null elements. When the values
buffer
// has no logical null elements there is nothing to remove, so every row is
// returned unchanged and the result is bit-identical to the input.
logical_null_count
// (not null_count) is used so a NullArray, whose elements are all logically
null,
// is counted correctly; NullArray::nulls() is None.
if values.logical_null_count() == 0 {
return Ok(Arc::new(list_array.clone()));
}
let value_nulls = values.logical_nulls();
```
This keeps the slow path byte-for-byte identical and removes the allocation
from the common case.
--
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]
