You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "pitrou (via GitHub)" <gi...@apache.org> on 2024/12/03 18:06:45 UTC
[PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
pitrou opened a new pull request, #44921:
URL: https://github.com/apache/arrow/pull/44921
### Rationale for this change
A non-nullable column that contains nulls would result in an invalid Parquet file, so we'd rather raise an error when writing.
This detection is only implemented for leaf columns. Implementing it for non-leaf columns would be more involved, and also doesn't actually seem necessary.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Raising a clear error when trying to write invalid data to Parquet, instead of letting the Parquet writer silently generate an invalid file.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1868570496
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
I just found that `Array::Validate()` and `Array::ValidateFull()` are checking `DataType` not `Field`. If it knows the latter, we can also validate null_count with nullable from that Field.
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
Should we take special care when it is `kUnknownNullCount`? It is possible for an end user to produce an array without setting a precise null_count value.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1868668174
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
> Should we take special care when it is kUnknownNullCount? It is possible for an end user to produce an array without setting a precise null_count value.
`kUnknownNullCount` would be counted when it's that value
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1868672995
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
Yes in most cases. I remember that when I did the trick to make mutable (and reusable) arrays. An empty ArrayData is first created by calling `std::shared_ptr<ArrayData> ArrayData::Make(std::shared_ptr<DataType> type, int64_t length, int64_t null_count, int64_t offset)` with default values and then set it to array via `void Array::SetData(const std::shared_ptr<ArrayData>& data)`. The null_count is -1 if not manually maintained.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #44921:
URL: https://github.com/apache/arrow/pull/44921#issuecomment-2517198211
@lidavidm @jduo Are the Java changes ok?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #44921:
URL: https://github.com/apache/arrow/pull/44921#issuecomment-2515256936
:warning: GitHub issue #41667 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1868967929
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
> I just found that `Array::Validate()` and `Array::ValidateFull()` are checking `DataType` not `Field`. If it knows the latter, we can also validate null_count with nullable from that Field.
See https://github.com/apache/arrow/issues/31387
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1869334175
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
As what parquet handle nulls, `leaf_field_nullable` might including nulls in parents?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #44921:
URL: https://github.com/apache/arrow/pull/44921#issuecomment-2518206218
After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit ded148c2ef5dd441a3b6ab9496d0ed0aeb940f71.
There were 132 benchmark results with an error:
- Commit Run on `arm64-t4g-2xlarge-linux` at [2024-12-04 13:46:24Z](https://conbench.ursa.dev/compare/runs/45a450c017a54b1788929762323a8e81...8095de3a997b45b69ff8164ee4b304aa/)
- [`tpch` (R) with engine=arrow, format=parquet, language=R, memory_map=False, query_id=TPCH-07, scale_factor=1](https://conbench.ursa.dev/benchmark-results/067506b6f93072a38000ffda261010b9)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-09, scale_factor=1](https://conbench.ursa.dev/benchmark-results/067506b96ab6790a800095856d00dd30)
- and 130 more (see the report linked below)
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/33930978171) has more details. It also includes information about 11 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #44921:
URL: https://github.com/apache/arrow/pull/44921#issuecomment-2516917218
Well, I think I got the failure fixed anyway (at least locally it works, let's wait for CI).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1869399059
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
`leaf_field_nullable` is computed from `PathBuilder.nullable_in_parent_`, which itself is initialized in e.g.:
https://github.com/apache/arrow/blob/fb8e8122f623f4548b22ece7485c4570d7ece1a6/cpp/src/parquet/arrow/path_internal.cc#L792-L801
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #44921:
URL: https://github.com/apache/arrow/pull/44921#issuecomment-2516557445
Java JNI failure looks related.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1869141754
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
I don't even know what `single_nullable_element` means.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1869395628
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
> As what parquet handle nulls, `leaf_field_nullable` might including nulls in parents?
Not according to my reading of the code.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #44921:
URL: https://github.com/apache/arrow/pull/44921#issuecomment-2516910444
The Java code has been moved, so we could ignore the JNI failure here and open an issue in the other repo (I'm still getting all the CI set up, though)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #44921:
URL: https://github.com/apache/arrow/pull/44921
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1868747841
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
```c++
int64_t ArrayData::GetNullCount() const {
int64_t precomputed = this->null_count.load();
if (ARROW_PREDICT_FALSE(precomputed == kUnknownNullCount)) {
if (this->buffers[0]) {
precomputed = this->length -
CountSetBits(this->buffers[0]->data(), this->offset, this->length);
} else {
precomputed = 0;
}
this->null_count.store(precomputed);
}
return precomputed;
}
```
`null_count()` call will generate null_count if not exists.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1868752003
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
I don't know should we check `single_nullable_element` rather than `leaf_field_nullable`
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1869416151
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
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
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1869401021
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
And for lists:
https://github.com/apache/arrow/blob/fb8e8122f623f4548b22ece7485c4570d7ece1a6/cpp/src/parquet/arrow/path_internal.cc#L725-L741
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-41667: [C++][Parquet] Refuse writing non-nullable column that contains nulls [arrow]
Posted by "mapleFU (via GitHub)" <gi...@apache.org>.
mapleFU commented on code in PR #44921:
URL: https://github.com/apache/arrow/pull/44921#discussion_r1869395665
##########
cpp/src/parquet/column_writer.cc:
##########
@@ -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) {
Review Comment:
E.g. `List< int notnull> nullable`, we may have the `single_nullable_element == false` but `leaf_field_nullable == true`?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org