Repository navigation
Conversation
|
|
| Result<std::shared_ptr<BooleanArray>> MakeDropNullFilter(const Array& values, | ||
| ExecContext* ctx) { | ||
| if (is_union(values.type_id())) { | ||
| ARROW_ASSIGN_OR_RAISE(auto validity, IsValid(values, ctx)); |
There was a problem hiding this comment.
What if Arrow is built without the optional Compute kernels? drop_null is available in core, but is_valid is not registered there, so wouldn't this fail?
Could we use the core union validity helpers directly and cover that configuration in a test?
There was a problem hiding this comment.
Confirmed: the new core-registry regression fails on the initial PR head with No function registered with name: is_valid.
Pushed 23fd210 to this PR's source branch, using the core sparse/dense union-validity helpers directly. The new test uses only the built-in registry and verifies that is_valid is absent. With ARROW_COMPUTE=OFF, the complete arrow-compute-internals-test suite now passes (73 tests). The full-compute selection suite passes (171 tests, one existing disabled test). The revision and validation were assisted by Codex.
The PR API is still reporting the old head immediately after the push; the linked commit is the revision validated above.
| return values; | ||
| } | ||
| if (values->null_count() == values->length()) { | ||
| if (null_count == values->length()) { |
There was a problem hiding this comment.
Scenario: all values are null and the union has a bool8 extension child. MakeEmptyArray fails because it needs a builder for the extension, no? Same for chunked path.
Can we cover both dense and sparse unions with extension children?
There was a problem hiding this comment.
Confirmed: the new bool8-child regression fails on the initial PR head with MakeBuilder: cannot construct builder for type extension<arrow.bool8>.
The same follow-up commit uses MakeArrayOfNull(type, 0) for empty unions across the array, chunked-array and record-batch paths; the table path uses the corrected batch handling. This avoids extension builders and retaining input buffers, while preserving the existing empty construction for non-union types.
Added dense/sparse coverage for all-null Array, ChunkedArray, RecordBatch and Table inputs, plus mixed values, slices and chunking. The full selection suite passes: 171 tests, one existing disabled test.
| return RecordBatch::MakeEmpty(batch->schema(), ctx->memory_pool()); | ||
| ArrayVector empty_columns; | ||
| for (const auto& column : batch->columns()) { | ||
| ARROW_ASSIGN_OR_RAISE(auto empty, MakeDropNullEmptyArray(column->type(), ctx)); |
There was a problem hiding this comment.
The extension fix misses a sibling-column case: an all-null union beside a valid list column containing [[], []].
Dropping all rows calls MakeEmptyArray for the list column, which fails with "MakeBuilder: cannot construct builder for type extension<arrow.bool8>".
Could we handle this case too and add dense/sparse regressions for RecordBatch and Table?
Rationale for this change
drop_nullchecks physical null counts and validity bitmaps, which miss the logical nulls in dense and sparse unions. As a result, it can retain null values or rows across Array, ChunkedArray, RecordBatch and Table inputs.This addresses the union portion of #43851. REE selection support remains outside this patch; it does not resolve the whole issue.
What changes are included in this PR?
Use logical null counts and core union-validity helpers across the four input kinds, without requiring the optional
is_validkernel. Empty union results use the null-array factory so extension children do not require builders or retain input buffers. Keep the existing physical-bitmap paths for other types, including dictionary arrays. AMayHaveLogicalNullscheck avoids scanning union values when their children cannot contain nulls.Regressions cover dense/sparse unions, non-default type codes, slices, empty/all-null inputs, chunk boundaries, and row filtering with both union and ordinary columns. They also cover core-only registration and bool8 extension children across all four input kinds.
Are these changes tested?
Windows x64, MSVC 19.44, Release, static Arrow/GTest 1.18.0 test linkage:
is_validwithARROW_COMPUTE=OFF, and an unsupported bool8 builder for all-null union results. Both pass after the follow-up fix.ARROW_COMPUTE=OFF,arrow-compute-internals-test: 73 passed.ARROW_COMPUTE=ON,arrow-compute-vector-selection-test: 171 passed, one pre-existing disabled test. Includes dense/sparse bool8 extension children for Array, ChunkedArray, RecordBatch and Table, plus mixed values and slices.clang-format18.1.8 andgit diff --check: pass.The original behavior was also reproduced with PyArrow 26.0.0, using explicit validity filtering and Python row selection as controls. The patched validation above is the native C++ suite.
Are there any user-facing changes?
Logical union nulls are now removed, including the corresponding full rows in RecordBatch/Table inputs.
This PR contains a "Critical Fix" under the incorrect-data criterion:
drop_nullpreviously returned rows/values that should have been removed. There are no public API signature changes.Was AI used for this PR?
Codex generated the implementation and regression tests, performed code review, and ran the local validation described above.
PR code and description written by:
Reviewed before submission by: