bkietz commented on a change in pull request #10817:
URL: https://github.com/apache/arrow/pull/10817#discussion_r678503252
##########
File path: cpp/src/arrow/scalar_test.cc
##########
@@ -930,117 +930,134 @@ TEST(TestDictionaryScalar, Cast) {
}
}
-TEST(TestSparseUnionScalar, Basics) {
- auto ty = sparse_union({field("string", utf8()), field("number", uint64())});
+void CheckGetValidUnionScalar(const Array& arr, int64_t index, const Scalar&
expected,
+ const Scalar& expected_value) {
+ ASSERT_OK_AND_ASSIGN(auto scalar, arr.GetScalar(index));
+ ASSERT_TRUE(scalar->Equals(expected));
+
+ const auto& as_union = checked_cast<const UnionScalar&>(*scalar);
+ ASSERT_TRUE(as_union.is_valid);
+ ASSERT_TRUE(as_union.value->Equals(expected_value));
+}
- auto alpha = MakeScalar("alpha");
- auto beta = MakeScalar("beta");
- ASSERT_OK_AND_ASSIGN(auto two, MakeScalar(uint64(), 2));
+void CheckGetNullUnionScalar(const Array& arr, int64_t index) {
+ ASSERT_OK_AND_ASSIGN(auto scalar, arr.GetScalar(index));
+ ASSERT_TRUE(scalar->Equals(MakeNullScalar(arr.type())));
- auto scalar_alpha = SparseUnionScalar(alpha, ty);
- auto scalar_beta = SparseUnionScalar(beta, ty);
- auto scalar_two = SparseUnionScalar(two, ty);
+ const auto& as_union = checked_cast<const UnionScalar&>(*scalar);
+ ASSERT_FALSE(as_union.is_valid);
+ // XXX in reality, the union array doesn't have a validity bitmap.
+ // Validity is inferred from the underlying child value, which should maybe
+ // be reflected here...
+ ASSERT_EQ(as_union.value, nullptr);
Review comment:
Sorry for my incorrect `suggestion`. Indeed: if we wanted to maintain
consistent "shape" there can be no such thing as a union scalar for which
`is_valid == false` since that would be analogous to a union array with a top
level null slot. Having `MakeNullSalar(union_type)->is_valid == true` is
counterintuitive, but it is consistent with the behavior of union arrays:
`MakeArrayOfNull(union_type, 1).ValueOrDie()->IsValid(0) == true` so I still
think it's the correct decision here.
I do think the caveat is counterintuitive enough to explicitly note in the
doccomments for MakeArrayOfNull and MakeNullScalar.
--
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]
