pitrou commented on code in PR #50546:
URL: https://github.com/apache/arrow/pull/50546#discussion_r3623554675
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -384,12 +385,48 @@ struct CastStruct {
const auto& in_field = in_type.field(in_field_index);
const auto& in_values =
(in_array.child_data[in_field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+
if (in_field->nullable() && !out_field->nullable() &&
in_values->GetNullCount() > 0) {
- return Status::Invalid(
- "field '", in_field->name(), "' of type ",
in_field->type()->ToString(),
- " has nulls. Can't cast to non-nullable field '",
out_field->name(),
- "' of type ", out_field_type->ToString());
+ bool has_nulls = false;
+ const uint8_t* parent_bitmap = in_array.buffers[0].data;
+ const uint8_t* child_bitmap = in_values->buffers.empty() ? nullptr :
(in_values->buffers[0] ? in_values->buffers[0]->data() : nullptr);
+
+ if (parent_bitmap == nullptr) {
+ // Parent has no nulls. Since child has nulls, they are unmasked.
+ has_nulls = true;
+ } else if (child_bitmap == nullptr) {
+ // Child has nulls but no bitmap (e.g. NullArray, RunEndEncoded,
Union).
+ // We must semantically check if any valid parent element
corresponds to a null child.
Review Comment:
The `BinaryBitBlockCounter` path below does not account for logical nulls,
so I don't think this one should, either.
We can just hardcode an error for NullArray.
##########
cpp/src/arrow/compute/kernels/scalar_cast_nested.cc:
##########
@@ -384,12 +385,48 @@ struct CastStruct {
const auto& in_field = in_type.field(in_field_index);
const auto& in_values =
(in_array.child_data[in_field_index].ToArrayData()->Slice(
in_array.offset, in_array.length));
+
if (in_field->nullable() && !out_field->nullable() &&
in_values->GetNullCount() > 0) {
- return Status::Invalid(
- "field '", in_field->name(), "' of type ",
in_field->type()->ToString(),
- " has nulls. Can't cast to non-nullable field '",
out_field->name(),
- "' of type ", out_field_type->ToString());
+ bool has_nulls = false;
+ const uint8_t* parent_bitmap = in_array.buffers[0].data;
+ const uint8_t* child_bitmap = in_values->buffers.empty() ? nullptr :
(in_values->buffers[0] ? in_values->buffers[0]->data() : nullptr);
+
+ if (parent_bitmap == nullptr) {
+ // Parent has no nulls. Since child has nulls, they are unmasked.
+ has_nulls = true;
+ } else if (child_bitmap == nullptr) {
+ // Child has nulls but no bitmap (e.g. NullArray, RunEndEncoded,
Union).
+ // We must semantically check if any valid parent element
corresponds to a null child.
+ for (int64_t i = 0; i < in_array.length; ++i) {
+ if (arrow::bit_util::GetBit(parent_bitmap, in_array.offset + i)
&&
+ in_values->IsNull(i)) {
+ has_nulls = true;
+ break;
+ }
+ }
+ } else {
+ // Both parent and child have bitmaps. Check if parent is valid
AND child is null.
+ arrow::internal::BinaryBitBlockCounter bit_counter(
+ parent_bitmap, in_array.offset,
+ child_bitmap, in_values->offset, in_array.length);
+ int64_t position = 0;
+ while (position < in_array.length) {
+ arrow::internal::BitBlockCount block =
bit_counter.NextAndNotWord();
+ if (block.popcount > 0) {
+ has_nulls = true;
+ break;
+ }
+ position += block.length;
+ }
+ }
Review Comment:
If we get here, I think we should make sure that the final casted child does
not have a null bitmap, otherwise it might violate expectations for a nullable
field (which are not well specified, unfortunately).
--
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]
