dalingmeng commented on code in PR #340:
URL: https://github.com/apache/paimon-cpp/pull/340#discussion_r4023077879
##########
src/paimon/common/utils/arrow/arrow_utils_test.cpp:
##########
@@ -220,6 +220,57 @@ TEST(ArrowUtilsTest, TestCheckNullableMatchWithStruct) {
}
}
+TEST(ArrowUtilsTest, TestCheckNullableMatchParentNullMasksChildren) {
+ auto child = arrow::field("child", arrow::int32(), /*nullable=*/false);
+ auto struct_field = arrow::field("parent", arrow::struct_({child}),
/*nullable=*/true);
+ auto vector_type =
+ arrow::fixed_size_list(arrow::field("item", arrow::float32(),
/*nullable=*/false), 3);
+ auto vector_field = arrow::field("embedding", vector_type,
/*nullable=*/true);
+ auto schema = arrow::schema({struct_field, vector_field});
+
+ // Do not build the complete batch with ArrayFromJSON: for a null STRUCT
it appends a valid
+ // default value to primitive children, which cannot represent the hidden
child null under test.
+ auto parent_with_validity = checked_pointer_cast<arrow::StructArray>(
+ arrow::ipc::internal::json::ArrayFromJSON(arrow::struct_({child}),
R"([null, [1]])")
+ .ValueOrDie());
+ std::shared_ptr<arrow::Array> child_array =
+ arrow::ipc::internal::json::ArrayFromJSON(arrow::int32(), "[null,
1]").ValueOrDie();
+ std::shared_ptr<arrow::StructArray> parent =
+ arrow::StructArray::Make({child_array}, {child},
parent_with_validity->null_bitmap(),
+ parent_with_validity->null_count())
+ .ValueOrDie();
+ auto vector = checked_pointer_cast<arrow::FixedSizeListArray>(
+ arrow::ipc::internal::json::ArrayFromJSON(vector_type, R"([null, [1.0,
2.0, 3.0]])")
+ .ValueOrDie());
+ std::shared_ptr<arrow::StructArray> batch =
+ arrow::StructArray::Make({parent, vector}, {struct_field,
vector_field}).ValueOrDie();
+
+ ASSERT_OK(ArrowUtils::CheckNullabilityMatch(schema, batch));
+}
+
+TEST(ArrowUtilsTest, TestCheckNullableMatchRejectsVisibleChildNulls) {
+ auto child = arrow::field("child", arrow::int32(), /*nullable=*/false);
+ auto struct_field = arrow::field("parent", arrow::struct_({child}),
/*nullable=*/true);
+ std::shared_ptr<arrow::Array> struct_batch =
+
arrow::ipc::internal::json::ArrayFromJSON(arrow::struct_({struct_field}),
R"([[[null]]])")
+ .ValueOrDie();
+ ASSERT_NOK_WITH_MSG(
+ ArrowUtils::CheckNullabilityMatch(arrow::schema({struct_field}),
struct_batch),
+ "CheckNullabilityMatch failed, field child not nullable while data
have null value");
+
+ auto vector_type =
+ arrow::fixed_size_list(arrow::field("item", arrow::float32(),
/*nullable=*/false), 3);
+ auto vector_field = arrow::field("embedding", vector_type,
/*nullable=*/true);
+ std::shared_ptr<arrow::Array> vector_batch =
+
arrow::ipc::internal::json::ArrayFromJSON(arrow::struct_({vector_field}),
+ R"([[[1.0, null, 3.0]]])")
+ .ValueOrDie();
+ ASSERT_NOK_WITH_MSG(
+ ArrowUtils::CheckNullabilityMatch(arrow::schema({vector_field}),
vector_batch),
+ "VECTOR field embedding is invalid: CheckNullabilityMatch failed,
field item not nullable "
+ "while data have null value");
+}
+
Review Comment:
[Tests] Add parent-null coverage for LIST / MAP / sliced / deeply-nestedThe
new TestCheckNullableMatchParentNullMasksChildren and
TestCheckNullableMatchRejectsVisibleChildNulls only cover STRUCT and
VECTOR(FSL). But the VisibleRanges fix also governs LIST/MAP, and the old
InnerCheckNullabilityMatch recursed into the whole list_array->values() for
LIST without excluding values hidden under a null parent row — one of the main
false-positive paths. Suggest adding these regression cases to lock in the fix:
LIST: a nullable list row is null while its value_field is declared
not-nullable, and the values in that parent-null row's range contain null →
should pass (no false positive); a null in a visible row → should be rejected.
MAP: parent map row is null, key/item not-nullable with nulls at the
corresponding positions → should pass.
Sliced array (offset ≠ 0): validate a sliced nested array to confirm
value_offset mapping and IsValid indexing remain correct.
Deeply nested (STRUCT > LIST > STRUCT): multi-level visible-range narrowing,
where a mid-level null parent masks deeper child nulls.
--
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]
