dmitry-chirkov-dremio commented on code in PR #54725:
URL: https://github.com/apache/arrow/pull/54725#discussion_r4337515019
##########
cpp/src/arrow/compute/kernels/vector_selection.cc:
##########
@@ -70,32 +72,51 @@ using TakeState = OptionsWrapper<TakeOptions>;
// ----------------------------------------------------------------------
// DropNull Implementation
-std::shared_ptr<arrow::BooleanArray> MakeDropNullFilter(const Array& values) {
+int64_t DropNullCount(const Array& values) {
+ if (is_union(values.type_id())) {
+ return values.data()->MayHaveLogicalNulls() ?
values.ComputeLogicalNullCount() : 0;
+ }
+ return values.null_count();
+}
+
+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));
Review 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?
##########
cpp/src/arrow/compute/kernels/vector_selection.cc:
##########
@@ -70,32 +72,51 @@ using TakeState = OptionsWrapper<TakeOptions>;
// ----------------------------------------------------------------------
// DropNull Implementation
-std::shared_ptr<arrow::BooleanArray> MakeDropNullFilter(const Array& values) {
+int64_t DropNullCount(const Array& values) {
+ if (is_union(values.type_id())) {
+ return values.data()->MayHaveLogicalNulls() ?
values.ComputeLogicalNullCount() : 0;
+ }
+ return values.null_count();
+}
+
+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));
+ return checked_pointer_cast<BooleanArray>(validity.make_array());
+ }
auto& bitmap_buffer = values.null_bitmap();
return std::make_shared<BooleanArray>(values.length(), bitmap_buffer,
nullptr, 0,
values.offset());
}
Result<Datum> DropNullArray(const std::shared_ptr<Array>& values, ExecContext*
ctx) {
- if (values->null_count() == 0) {
+ const auto null_count = DropNullCount(*values);
+ if (null_count == 0) {
return values;
}
- if (values->null_count() == values->length()) {
+ if (null_count == values->length()) {
Review 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?
--
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]
