From fe9f76b72b543473384f6495f9966353199e9a74 Mon Sep 17 00:00:00 2001 From: Zehua Zou Date: Fri, 23 Oct 2026 18:55:09 +0800 Subject: [PATCH 1/2] Allow masked nulls in required fields when writing --- .../parquet/arrow/arrow_reader_writer_test.cc | 42 +++++++++++++++++++ cpp/src/parquet/arrow/path_internal.cc | 4 +- cpp/src/parquet/column_writer.cc | 23 +++++++++- 3 files changed, 66 insertions(+), 3 deletions(-) diff --git a/cpp/src/parquet/arrow/arrow_reader_writer_test.cc b/cpp/src/parquet/arrow/arrow_reader_writer_test.cc index d858962d4728..b73638d43085 100644 --- a/cpp/src/parquet/arrow/arrow_reader_writer_test.cc +++ b/cpp/src/parquet/arrow/arrow_reader_writer_test.cc @@ -3262,6 +3262,44 @@ TEST(ArrowReadWrite, NestedRequiredField) { /*row_group_size=*/8); } +TEST(ArrowReadWrite, NestedRequiredFieldWithMaskedNulls) { + auto type = + ::arrow::struct_({::arrow::field("inner", ::arrow::int32(), /*nullable=*/false)}); + auto values = ::arrow::ArrayFromJSON(type, R"([{"inner": 42}])"); + auto indices = ::arrow::ArrayFromJSON(::arrow::int32(), "[0, 0, null, 0]"); + ASSERT_OK_AND_ASSIGN(auto array, ::arrow::compute::Take(*values, *indices)); + + const auto& structs = checked_cast(*array); + ASSERT_EQ(structs.null_count(), 1); + ASSERT_EQ(structs.field(0)->null_count(), 1); + + auto table = ::arrow::Table::Make( + ::arrow::schema({::arrow::field("outer", type, /*nullable=*/true)}), {array}); + ASSERT_OK(table->ValidateFull()); + CheckSimpleRoundtrip(table, /*row_group_size=*/4); + + auto offsets = ::arrow::ArrayFromJSON(::arrow::int32(), "[null, 0, 0, 4]"); + ASSERT_OK_AND_ASSIGN(auto lists, ::arrow::ListArray::FromArrays(*offsets, *array)); + auto list_table = ::arrow::Table::Make( + ::arrow::schema({::arrow::field("outer", lists->type())}), {std::move(lists)}); + ASSERT_OK(list_table->ValidateFull()); + // Round-trip null, empty, and non-empty lists in one row group. + CheckSimpleRoundtrip(list_table, /*row_group_size=*/3); + + // Make all parent rows valid while keeping the child null. + ASSERT_OK_AND_ASSIGN(auto unmasked, + ::arrow::StructArray::Make(structs.fields(), type->fields())); + auto invalid_table = ::arrow::Table::Make(table->schema(), {std::move(unmasked)}); + // Also cover an all-null child array. + for (const auto& invalid_slice : {invalid_table, invalid_table->Slice(2, 1)}) { + ARROW_SCOPED_TRACE("num_rows = ", invalid_slice->num_rows()); + auto sink = CreateOutputStream(); + EXPECT_RAISES_WITH_MESSAGE_THAT( + Invalid, ::testing::HasSubstr("is declared non-nullable but contains nulls"), + WriteTable(*invalid_slice, ::arrow::default_memory_pool(), sink)); + } +} + TEST(ArrowReadWrite, Decimal) { using ::arrow::field; @@ -3843,6 +3881,10 @@ TEST(TestArrowWrite, InvalidSchema) { CheckWritingNonNullableColumnWithNulls( ::arrow::field("a", ::arrow::utf8(), /*nullable=*/false), R"([{"a": "foo"}, {"a": null}])"); + CheckWritingNonNullableColumnWithNulls( + ::arrow::field("a", ::arrow::list(::arrow::field("item", ::arrow::int32(), + /*nullable=*/false))), + R"([{"a": null}, {"a": []}, {"a": [42, null]}])"); } void DoNestedValidate(const std::shared_ptr<::arrow::DataType>& inner_type, diff --git a/cpp/src/parquet/arrow/path_internal.cc b/cpp/src/parquet/arrow/path_internal.cc index c626dcbf48b6..48e054e145a0 100644 --- a/cpp/src/parquet/arrow/path_internal.cc +++ b/cpp/src/parquet/arrow/path_internal.cc @@ -724,7 +724,9 @@ class PathBuilder { // and the array does in fact contain nulls, we will end up // traversing the null bitmap twice (once here and once when calculating // rep/def levels). - if (LazyNoNulls(array)) { + // Required leaf nulls must not lower definition levels: they may be masked by + // an ancestor, but otherwise WriteArrow() must reject them as invalid. + if (!nullable_in_parent_ || LazyNoNulls(array)) { info_.path.emplace_back(AllPresentTerminalNode{info_.max_def_level}); } else if (LazyNullCount(array) == array.length()) { info_.path.emplace_back(AllNullsTerminalNode(info_.max_def_level - 1)); diff --git a/cpp/src/parquet/column_writer.cc b/cpp/src/parquet/column_writer.cc index 3b4a873b5192..9d5b3b7bcd08 100644 --- a/cpp/src/parquet/column_writer.cc +++ b/cpp/src/parquet/column_writer.cc @@ -1430,8 +1430,27 @@ class TypedColumnWriterImpl : public ColumnWriterImpl, (level_info_.def_level == level_info_.repeated_ancestor_def_level + 1) && leaf_field_nullable; if (!leaf_field_nullable && leaf_array.null_count() != 0) { - return Status::Invalid("Column '", descr_->name(), - "' is declared non-nullable but contains nulls"); + bool has_unmasked_nulls = true; + if (level_info_.HasNullableValues()) { + has_unmasked_nulls = false; + int64_t leaf_index = 0; + for (int64_t i = 0; i < num_levels; ++i) { + // Null or empty lists have no corresponding slot in the leaf array. + if (def_levels[i] < level_info_.repeated_ancestor_def_level) { + continue; + } + // A required leaf only needs to be valid when all ancestors are present. + if (def_levels[i] == level_info_.def_level && leaf_array.IsNull(leaf_index)) { + has_unmasked_nulls = true; + break; + } + ++leaf_index; + } + } + if (has_unmasked_nulls) { + return Status::Invalid("Column '", descr_->name(), + "' is declared non-nullable but contains nulls"); + } } bool maybe_parent_nulls = level_info_.HasNullableValues() && !single_nullable_element; if (maybe_parent_nulls) { From 50af3fc2c08b6c20156a0c6785489077cd747883 Mon Sep 17 00:00:00 2001 From: Zehua Zou Date: Fri, 23 Oct 2026 21:23:51 +0800 Subject: [PATCH 2/2] fix ci --- cpp/src/parquet/arrow/arrow_reader_writer_test.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cpp/src/parquet/arrow/arrow_reader_writer_test.cc b/cpp/src/parquet/arrow/arrow_reader_writer_test.cc index b73638d43085..4756628baea9 100644 --- a/cpp/src/parquet/arrow/arrow_reader_writer_test.cc +++ b/cpp/src/parquet/arrow/arrow_reader_writer_test.cc @@ -3279,9 +3279,11 @@ TEST(ArrowReadWrite, NestedRequiredFieldWithMaskedNulls) { CheckSimpleRoundtrip(table, /*row_group_size=*/4); auto offsets = ::arrow::ArrayFromJSON(::arrow::int32(), "[null, 0, 0, 4]"); - ASSERT_OK_AND_ASSIGN(auto lists, ::arrow::ListArray::FromArrays(*offsets, *array)); + ASSERT_OK_AND_ASSIGN(auto list_array, ::arrow::ListArray::FromArrays(*offsets, *array)); + auto list_type = list_array->type(); auto list_table = ::arrow::Table::Make( - ::arrow::schema({::arrow::field("outer", lists->type())}), {std::move(lists)}); + ::arrow::schema({::arrow::field("outer", std::move(list_type))}), + {std::move(list_array)}); ASSERT_OK(list_table->ValidateFull()); // Round-trip null, empty, and non-empty lists in one row group. CheckSimpleRoundtrip(list_table, /*row_group_size=*/3);