Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls #44921

Merged
merged 2 commits into from
Dec 4, 2024
Merged

GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls #44921

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
450410a
GH-41667: [C++][Parquet] Refuse writing non-nullable column that cont…
pitrou Dec 3, 2024
bb71157
Try fix for JNI test failure
pitrou Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions cpp/src/parquet/arrow/arrow_reader_writer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3349,6 +3349,27 @@ TEST(TestArrowWrite, CheckChunkSize) {
WriteTable(*table, ::arrow::default_memory_pool(), sink, chunk_size));
}

void CheckWritingNonNullableColumnWithNulls(std::shared_ptr<::arrow::Field> field,
std::string json_batch) {
ARROW_SCOPED_TRACE("field = ", field, ", json_batch = ", json_batch);
auto schema = ::arrow::schema({field});
auto table = ::arrow::TableFromJSON(schema, {json_batch});
auto sink = CreateOutputStream();
EXPECT_RAISES_WITH_MESSAGE_THAT(
Invalid, ::testing::HasSubstr("is declared non-nullable but contains nulls"),
WriteTable(*table, ::arrow::default_memory_pool(), sink));
}

TEST(TestArrowWrite, InvalidSchema) {
// GH-41667: nulls in non-nullable column
CheckWritingNonNullableColumnWithNulls(
::arrow::field("a", ::arrow::int32(), /*nullable=*/false),
R"([{"a": 456}, {"a": null}])");
CheckWritingNonNullableColumnWithNulls(
::arrow::field("a", ::arrow::utf8(), /*nullable=*/false),
R"([{"a": "foo"}, {"a": null}])");
}

void DoNestedValidate(const std::shared_ptr<::arrow::DataType>& inner_type,
const std::shared_ptr<::arrow::Field>& outer_field,
const std::shared_ptr<Buffer>& buffer,
Expand Down
4 changes: 4 additions & 0 deletions cpp/src/parquet/column_writer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1301,6 +1301,10 @@ class TypedColumnWriterImpl : public ColumnWriterImpl, public TypedColumnWriter<
bool single_nullable_element =
(level_info_.def_level == level_info_.repeated_ancestor_def_level + 1) &&
leaf_field_nullable;
if (!leaf_field_nullable && leaf_array.null_count() != 0) {
pitrou marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know should we check single_nullable_element rather than leaf_field_nullable

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't even know what single_nullable_element means.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As what parquet handle nulls, leaf_field_nullable might including nulls in parents?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As what parquet handle nulls, leaf_field_nullable might including nulls in parents?

Not according to my reading of the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

E.g. List< int notnull> nullable, we may have the single_nullable_element == false but leaf_field_nullable == true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leaf_field_nullable is computed from PathBuilder.nullable_in_parent_, which itself is initialized in e.g.:

arrow/cpp/src/parquet/arrow/path_internal.cc

Lines 792 to 801 in fb8e812

Status Visit(const ::arrow::StructArray& array) {
MaybeAddNullable(array);
PathInfo info_backup = info_;
for (int x = 0; x < array.num_fields(); x++) {
nullable_in_parent_ = array.type()->field(x)->nullable();
RETURN_NOT_OK(VisitInline(*array.field(x)));
info_ = info_backup;
}
return Status::OK();
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for lists:

arrow/cpp/src/parquet/arrow/path_internal.cc

Lines 725 to 741 in fb8e812

template <typename T>
::arrow::enable_if_t<std::is_same<::arrow::ListArray, T>::value ||
std::is_same<::arrow::LargeListArray, T>::value,
Status>
Visit(const T& array) {
MaybeAddNullable(array);
// Increment necessary due to empty lists.
info_.max_def_level++;
info_.max_rep_level++;
// raw_value_offsets() accounts for any slice offset.
ListPathNode<VarRangeSelector<typename T::offset_type>> node(
VarRangeSelector<typename T::offset_type>{array.raw_value_offsets()},
info_.max_rep_level, info_.max_def_level - 1);
info_.path.emplace_back(std::move(node));
nullable_in_parent_ = array.list_type()->value_field()->nullable();
return VisitInline(*array.values());
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, there should be leaf_field_nullable. single_nullable_element is for preparing validity child buffer for List< int notnull> nullable. I got it wrong here

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) {
ARROW_ASSIGN_OR_RAISE(
Expand Down
12 changes: 7 additions & 5 deletions java/dataset/src/test/java/org/apache/arrow/dataset/TestAllTypes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,9 @@ private VectorSchemaRoot generateAllTypesVector(BufferAllocator allocator) {
// DenseUnion
List<Field> childFields = new ArrayList<>();
childFields.add(
new Field(
"int-child", new FieldType(false, new ArrowType.Int(32, true), null, null), null));
new Field("int-child", FieldType.notNullable(new ArrowType.Int(32, true)), null));
Field structField =
new Field(
"struct", new FieldType(true, ArrowType.Struct.INSTANCE, null, null), childFields);
new Field("struct", FieldType.nullable(ArrowType.Struct.INSTANCE), childFields);
Field[] fields =
new Field[] {
Field.nullablePrimitive("null", ArrowType.Null.INSTANCE),
Expand Down Expand Up @@ -239,7 +237,11 @@ private VectorSchemaRoot generateAllTypesVector(BufferAllocator allocator) {
largeListWriter.integer().writeInt(1);
largeListWriter.endList();

((StructVector) root.getVector("struct")).getChild("int-child", IntVector.class).set(1, 1);
var intChildVector =
((StructVector) root.getVector("struct")).getChild("int-child", IntVector.class);
// set a non-null value at index 0 since the field is not nullable
intChildVector.set(0, 0);
intChildVector.set(1, 1);
return root;
}

Expand Down
Loading