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?
There was a problem hiding this comment.
Confirmed on 23fd210: both new RecordBatch/Table regressions fail with MakeBuilder: cannot construct builder for type extension<arrow.bool8>.
Pushed 295917f. Empty outputs now use the null-array factory for extension-bearing sibling columns as well as unions. This constructs fresh empty buffers without requiring the nested extension builder. Ordinary types and the existing REE construction path remain unchanged.
The regressions pair an all-null dense/sparse union with a valid list<bool8> column containing [[], []], and check that dropping all rows preserves the schema and list/extension types for both RecordBatch and Table. Both new tests pass; the complete vector-selection suite passes (173 tests, one pre-existing disabled test). Formatting and git diff --check also pass. This revision and validation were assisted by Codex.
dmitry-chirkov-dremio
left a comment
dmitry-chirkov-dremio
left a comment
There was a problem hiding this comment.
I'm happy with what I see.
Ran an independent test locally covering 60 combinations of dense/sparse unions, RecordBatch/Table inputs and extension-bearing sibling types; all passed.
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 and extension-bearing sibling columns use the null-array factory so supported extension children do not require builders or retain input buffers. Run-end-encoded construction remains unchanged. 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, plus valid list sibling columns when all RecordBatch/Table rows are dropped.
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.23fd210:ARROW_COMPUTE=OFF,arrow-compute-internals-test: 73 passed.ARROW_COMPUTE=ON,arrow-compute-vector-selection-test: 173 passed, one pre-existing disabled test. Includes dense/sparse bool8 extension children for Array, ChunkedArray, RecordBatch and Table, plus mixed values, slices and list sibling columns. The two new RecordBatch/Table sibling-column regressions fail on23fd210with an unsupported nested bool8 builder and pass on295917f.clang-format18.1.5 andgit diff --check: pass on the latest revision.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: