ARROW-9603: [C++] Fix parquet write to not assume leaf-array validity bitmaps have the same values as parent structs #8219

emkornfield · 2020-09-18T09:21:35Z

Don't rely on nullability values of leaf nodes matching their parents.

In general it feels like the WriteArrow code path in column_writer.cc could use some cleanup to remove duplicated code, but while ugly I think this fix works.

github-actions · 2020-09-18T09:23:50Z

https://issues.apache.org/jira/browse/ARROW-9603

emkornfield · 2020-09-19T22:14:13Z

There is a better solution. I'll update the PR

emkornfield · 2020-09-20T00:52:41Z

Nm, I think this is likely the only reasonable approach. We might consider pushing bitmap building up the stack at some point.

wesm · 2020-09-21T17:10:52Z

I'm not sure I have enough mental context to review this PR carefully

emkornfield · 2020-09-21T18:21:38Z

@wesm hmm, I think you might be the only once familiar with the write path. But maybe @pitrou could give a more general review? Anyone else you would suggest?

emkornfield · 2020-09-21T18:22:48Z

cpp/src/parquet/arrow/path_internal.cc

@@ -838,10 +841,13 @@ class PathBuilder {
 #undef NOT_IMPLEMENTED_VISIT
  std::vector<PathInfo>& paths() { return paths_; }

+  bool root_is_nullable() const { return root_is_nullable_; }


this is unused now.

wesm · 2020-09-21T20:50:56Z

@xhochy might be the only one. I can do my best to provide some comments

wesm

I reviewed only the new parts -- overall seemed pretty reasonable. Can you update the PR title to explain the issue?

It's regrettable that this change has to touch so much code -- makes me think there could be some code restructurings possible in column_writer.cc, but not sure it's worth the expense right now

wesm · 2020-09-21T23:48:33Z

cpp/src/parquet/arrow/arrow_reader_writer_test.cc

+      ::arrow::Table::Make(
+          ::arrow::schema({struct_field}),
+          {std::make_shared<::arrow::ChunkedArray>(::arrow::MakeArray(struct_data))}),
+      /*row_group_size=*/8);


Seems like there might be a helper function opportunity if this pattern is repeated in other test functions

it turns out this could be simplified as well, so I don't think a helper function is necessary.

wesm · 2020-09-21T23:49:39Z

cpp/src/parquet/arrow/arrow_reader_writer_test.cc

+  auto struct_data = std::make_shared<ArrayData>(
+      struct_field->type(), /*length=*/8,
+      std::vector<std::shared_ptr<Buffer>>{validity_bitmap},
+      std::vector<std::shared_ptr<ArrayData>>{int_array->data()});


You can use ArrayData::Make for nicer syntax (don't have to write out std::vector<std::shared_ptr<Buffer>>)

thanks, I somehow keep forgetting this.

wesm · 2020-09-21T23:49:56Z

cpp/src/parquet/arrow/path_internal.cc

@@ -871,6 +877,8 @@ class MultipathLevelBuilderImpl : public MultipathLevelBuilder {
                     std::move(write_leaf_callback));
  }

+  bool Nested() const override { return !data_->child_data.empty(); }


wesm · 2020-09-21T23:51:31Z

cpp/src/parquet/arrow/writer.cc

-                                               ctx);
+              PARQUET_CATCH_AND_RETURN(column_writer->WriteArrow(
+                  result.def_levels, result.rep_levels, result.def_rep_level_count,
+                  *values_array, ctx, level_builder->Nested(), result.leaf_is_nullable));


Since WriteArrow returns Status, should we adopt that APIs must either return Status or throw an exception, but not both? (FWIW I regret that we chose to allow exceptions in the Parquet C++ project back in 2016)

done. I suppose it is too late to revisit this? Perhaps provide status/result returning methods in one PR and then deprecated exception throwing ones?

wesm · 2020-09-22T02:02:09Z

cpp/src/parquet/column_writer.cc

+    bool leaf_nulls_are_canonical =
+        (level_info_.def_level == level_info_.repeated_ancestor_def_level + 1) &&
+        array_nullable;
+    bool maybe_has_nulls = nested && !(leaf_is_not_nullable || leaf_nulls_are_canonical);


The fact that maybe_has_nulls is false whenever nested is false seems odd

yeah, I renamed maybe_has_nulls to maybe_has_parent_nulls which is hopefully clearer? Happy to pick another name that makes sense.

wesm · 2020-09-22T02:09:56Z

cpp/src/parquet/column_writer.cc

+    buffers[0] = bits_buffer_;
+    DCHECK(array->num_fields() == 0);
+    return arrow::MakeArray(std::make_shared<ArrayData>(
+        array->type(), array->length(), std::move(buffers), new_null_count));


Might be useful someday to have a helper function to make an array copy with a particular buffer replaced, I seem to recall a JIRA issue about this

Agree, looks like:https://issues.apache.org/jira/browse/ARROW-7071 might be it?

wesm · 2020-09-22T02:15:04Z

cpp/src/parquet/column_writer.cc

+    *null_count = io.null_count;
+  }
+
+  std::shared_ptr<Array> MaybeUpdateArray(std::shared_ptr<Array> array,


MaybeReplaceValidity?

…parents

emkornfield

@wesm thanks for the review. I addressed comments and rebased off of master to remove the first commit.

emkornfield · 2020-09-22T04:16:07Z

cpp/src/parquet/arrow/path_internal.cc

@@ -871,6 +877,8 @@ class MultipathLevelBuilderImpl : public MultipathLevelBuilder {
                     std::move(write_leaf_callback));
  }

+  bool Nested() const override { return !data_->child_data.empty(); }


emkornfield · 2020-09-22T04:17:47Z

cpp/src/parquet/arrow/writer.cc

-                                               ctx);
+              PARQUET_CATCH_AND_RETURN(column_writer->WriteArrow(
+                  result.def_levels, result.rep_levels, result.def_rep_level_count,
+                  *values_array, ctx, level_builder->Nested(), result.leaf_is_nullable));


done. I suppose it is too late to revisit this? Perhaps provide status/result returning methods in one PR and then deprecated exception throwing ones?

emkornfield · 2020-09-22T04:20:07Z

cpp/src/parquet/column_writer.cc

+    bool leaf_nulls_are_canonical =
+        (level_info_.def_level == level_info_.repeated_ancestor_def_level + 1) &&
+        array_nullable;
+    bool maybe_has_nulls = nested && !(leaf_is_not_nullable || leaf_nulls_are_canonical);


yeah, I renamed maybe_has_nulls to maybe_has_parent_nulls which is hopefully clearer? Happy to pick another name that makes sense.

emkornfield · 2020-09-22T04:21:11Z

cpp/src/parquet/column_writer.cc

+    *null_count = io.null_count;
+  }
+
+  std::shared_ptr<Array> MaybeUpdateArray(std::shared_ptr<Array> array,


emkornfield · 2020-09-22T04:23:34Z

cpp/src/parquet/column_writer.cc

+    buffers[0] = bits_buffer_;
+    DCHECK(array->num_fields() == 0);
+    return arrow::MakeArray(std::make_shared<ArrayData>(
+        array->type(), array->length(), std::move(buffers), new_null_count));


Agree, looks like:https://issues.apache.org/jira/browse/ARROW-7071 might be it?

emkornfield · 2020-09-22T04:33:32Z

@xhochy did you want to review?

pitrou · 2020-09-22T08:26:51Z

cpp/src/parquet/column_writer.cc

+                    ArrowWriteContext* ctx, bool nested, bool array_nullable) override {
+    BEGIN_PARQUET_CATCH_EXCEPTIONS
+    bool leaf_is_not_nullable = !level_info_.HasNullableValues();
+    // Leaf nulls are canonical when there is only a single null element and it is at the


"single nullable element" perhaps?

pitrou · 2020-09-22T08:28:01Z

cpp/src/parquet/column_writer.cc

+    // leaf.
+    bool leaf_nulls_are_canonical =
+        (level_info_.def_level == level_info_.repeated_ancestor_def_level + 1) &&
+        array_nullable;


array_nullable refers to the parent, the root, the leaf? This is difficult to follow.

Perhaps rename to parent_nullable or root_nullable or...

it is the leaf, will do some renaming to make this clearer.

pitrou · 2020-09-22T08:28:17Z

cpp/src/parquet/column_writer.cc

-                    ArrowWriteContext* ctx) override {
+                    ArrowWriteContext* ctx, bool nested, bool array_nullable) override {
+    BEGIN_PARQUET_CATCH_EXCEPTIONS
+    bool leaf_is_not_nullable = !level_info_.HasNullableValues();


Maybe avoid double negatives?

will rename.

pitrou · 2020-09-22T08:29:50Z

cpp/src/parquet/column_writer.cc

+        (level_info_.def_level == level_info_.repeated_ancestor_def_level + 1) &&
+        array_nullable;
+    bool maybe_parent_nulls =
+        nested && !(leaf_is_not_nullable || leaf_nulls_are_canonical);


Wait, if nested is false, is all this complicated dance required?

nested is actually unncessary. i've removed it. The only thing that matters is if the column is nullable according to columninfo and it isn't the only nullable column.

pitrou · 2020-09-22T08:30:35Z

cpp/src/parquet/column_writer.cc

+          arrow::AllocateResizableBuffer(
+              BitUtil::BytesForBits(properties_->write_batch_size()), ctx->memory_pool));
+      bits_buffer_->ZeroPadding();
+      std::static_pointer_cast<ResizableBuffer>(AllocateBuffer(allocator_, 0));


Is this allocating a new (temporary?) validity buffer for each write batch?

this line should be removed. but above, yes, we do allocate a new buffer for each WriteArrow call. I think the lifecycle of this object might only be used for one WriteArrow call. internally there is a concept of batching, and the allocation should only happen once for here for each of those batches.

cpp/src/parquet/column_writer.cc

xhochy · 2020-09-22T09:29:42Z

I reserved my self an hour tomorrow to review this. I haven't touched this code for over a year but this is the code path that actually got me into Arrow/Parquet project, so I'm happy to carve out time for it.

xhochy · 2020-09-23T14:58:43Z

cpp/src/parquet/arrow/arrow_reader_writer_test.cc

      3);
 }

+TEST(ArrowReadWrite, NestedRequiredField) {


The test cases look very very similar, just the name and the used values differ. I would have expected that we also would have set nullable=false somewhere in this one.

Are you looking at the latest version?

Right below this comment is:

auto int_field = ::arrow::field("int_array", ::arrow::int32(), /*nullable=*/false);

(note the last parameter?)

Ah, didn't see that when reviewing this code. Now this makes sense!

pitrou

+1 from me.

emkornfield requested a review from wesm September 18, 2020 09:21

emkornfield mentioned this pull request Sep 21, 2020

ARROW-8494: [C++][Parquet] Full support for reading mixed list and structs #8177

Closed

wesm changed the title ~~ARROW-9603: Fix parquet write~~ ARROW-9603: [C++] Fix parquet write Sep 21, 2020

emkornfield commented Sep 21, 2020

View reviewed changes

wesm reviewed Sep 22, 2020

View reviewed changes

emkornfield changed the title ~~ARROW-9603: [C++] Fix parquet write~~ ARROW-9603: [C++] Fix parquet write to not assume leaf-array validity bitmaps have the same values as parent structs Sep 22, 2020

emkornfield added 2 commits September 22, 2020 03:55

ARROW-9603: Don't rely on nested fields matching nullabilit of their …

08469ff

…parents

address PR comments

96d2ad5

emkornfield force-pushed the fix_parquet_write branch from 26745a9 to 96d2ad5 Compare September 22, 2020 04:32

emkornfield commented Sep 22, 2020

View reviewed changes

pitrou reviewed Sep 22, 2020

View reviewed changes

cpp/src/parquet/column_writer.cc Show resolved Hide resolved

emkornfield added 2 commits September 22, 2020 15:55

cleanup

5f9fff1

add more documentation

40e30bd

xhochy reviewed Sep 23, 2020

View reviewed changes

pitrou approved these changes Sep 24, 2020

View reviewed changes

pitrou closed this in 04eb733 Sep 24, 2020

asfimport mentioned this pull request Sep 25, 2020

[C++][Parquet] Write Arrow relies on unspecified behavior for nested types #25665

Closed

ARROW-9603: [C++] Fix parquet write to not assume leaf-array validity bitmaps have the same values as parent structs #8219

ARROW-9603: [C++] Fix parquet write to not assume leaf-array validity bitmaps have the same values as parent structs #8219

Uh oh!

Conversation

emkornfield commented Sep 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 18, 2020

Uh oh!

emkornfield commented Sep 19, 2020

Uh oh!

emkornfield commented Sep 20, 2020

Uh oh!

wesm commented Sep 21, 2020

Uh oh!

emkornfield commented Sep 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Sep 21, 2020

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emkornfield left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emkornfield commented Sep 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

emkornfield commented Sep 18, 2020 •

edited

Loading

pitrou Sep 22, 2020 •

edited

Loading