GH-45283: [C++][Parquet] Omit level histogram when max level is 0 #45285

wgtmac · 2025-01-16T16:06:27Z

Rationale for this change

The level histogram of size statistics can be omitted if its max level is 0. We haven't implemented this yet and enforces histogram size to be equal to max_level + 1. However, when reading a Parquet file with omitted level histogram, exception will be thrown.

What changes are included in this PR?

Omit level histogram when max level is 0.

Are these changes tested?

Yes, a test case has been added to reflect the change.

Are there any user-facing changes?

No.

GitHub Issue: [Python][C++][Parquet] "OSError: Repetition level histogram size mismatch" when reading parquet file in pyarrow since 19.0.0 #45283

github-actions · 2025-01-16T16:06:57Z

⚠️ GitHub issue #45283 has been automatically assigned in GitHub to PR creator.

wgtmac · 2025-01-16T16:07:12Z

cc @etseidl

etseidl · 2025-01-16T16:24:56Z

cpp/src/parquet/column_writer.cc

@@ -1609,6 +1609,9 @@ class TypedColumnWriterImpl : public ColumnWriterImpl, public TypedColumnWriter<

    auto add_levels = [](std::vector<int64_t>& level_histogram,
                         ::arrow::util::span<const int16_t> levels, int16_t max_level) {
+      if (max_level == 0) {


minor nit: should this be a constant?

IMHO this is already clear.

etseidl · 2025-01-16T16:28:03Z

cpp/src/parquet/size_statistics.cc

+  auto validate_histogram = [](const std::vector<int64_t>& histogram, int16_t max_level,
+                               const std::string& name) {
+    if (max_level == 0 && histogram.empty()) {
+      return;
+    }
+    if (histogram.size() != static_cast<size_t>(max_level + 1)) {
+      throw ParquetException(name + " level histogram size mismatch");
+    }
+  };
+  validate_histogram(repetition_level_histogram, descr->max_repetition_level(),
+                     "Repetition");
+  validate_histogram(definition_level_histogram, descr->max_definition_level(),
+                     "Definition");


You may want to add a threshold argument to validate_histogram, the reason being that the definition level histogram can be omitted for max def equal 0 or 1. arrow-rs and cudf both use the same cutoff of 0 on write (not sure what parquet-java does, but you wrote that 😄), but down the road someone might stick to the spec and this test will still fail.

etseidl

Oof...it's still too early for my brain to work github >.<

Anyway, this looks good to me on the write side, but the read side needs to account for the asymmetry in the spec. Thanks!

etseidl · 2025-01-16T16:31:46Z

cpp/src/parquet/size_statistics_test.cc

@@ -376,4 +376,22 @@ TEST_F(SizeStatisticsRoundTripTest, LargePage) {
                                                        /*byte_array_bytes=*/{90000}}));
 }

+TEST_F(SizeStatisticsRoundTripTest, MaxLevelZero) {


Should we add a file to parquet-testing that has max_def==1 and omits the def histogram?

I don't think if any open-sourced Parquet writer is able to create this yet...

I could coerce one into doing so. But with the change allowing empty histograms, it's probably not worth creating one now.

raulcd · 2025-01-16T16:45:32Z

but the read side needs to account for the asymmetry in the spec.

I am just following the PR, what do you mean by this? I am not sure I understand what case you're describing.

etseidl · 2025-01-16T17:04:37Z

but the read side needs to account for the asymmetry in the spec.

I am just following the PR, what do you mean by this? I am not sure I understand what case you're describing.

@raulcd The parquet spec says the repetition level histogram can be omitted if max_rep_level==0, but for the definition levels it can be omitted for max_def_level == 0 or 1. https://github.com/apache/parquet-format/blob/a498aa9a377edcdbc5da802cf9f1763a2e409411/src/main/thrift/parquet.thrift#L231-L237

So the case I'm describing is a file with a column with max_def_level==1, where the repetition histogram is populated but the definition histogram is not.

pitrou · 2025-01-16T17:09:33Z

The spec doesn't say anything about when it's allowed to be omitted.

It says in which situations it can be omitted without loss of information.

So we should assume that it can always be omitted, regardless of the max level values.

cpp/src/parquet/size_statistics.cc

pitrou · 2025-01-16T17:16:08Z

cpp/src/parquet/size_statistics_test.cc

+            ::arrow::TableFromJSON(schema, {R"([["foo"],["bar"]])"}),
+            /*max_row_group_length=*/2,
+            /*page_size=*/1024);
+  ReadSizeStatistics();


Does it also read the full file? We should ensure we don't crash elsewhere.

ReadSizeStatistics currently only reads the metadata where it crashes from the issue. Let me add a new function to verify scanning data.

mapleFU

So this patch omit the levels when writing and loose the validation on reading? I forget a bit can we always omit the level whatever the max_def_level is...

etseidl · 2025-01-17T05:39:58Z

cpp/src/parquet/size_statistics.cc

+  auto validate_histogram = [](const std::vector<int64_t>& histogram, int16_t max_level,
+                               const std::string& name) {
+    if (histogram.empty()) {
+      // A levels histogram is always allowed to be missing.


Should we also allow for missing unencoded_byte_array_data_bytes below? Maybe a different PR.

Yes, I just thought about this and will loose it as well.

apacheGH-45283: [C++][Parquet] Omit level histogram when max level is 0

6959c5b

github-actions bot added Component: Parquet Component: C++ awaiting review Awaiting review labels Jan 16, 2025

wgtmac requested a review from pitrou January 16, 2025 16:07

etseidl reviewed Jan 16, 2025

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 16, 2025

etseidl reviewed Jan 16, 2025

View reviewed changes

pitrou reviewed Jan 16, 2025

View reviewed changes

cpp/src/parquet/size_statistics.cc Outdated Show resolved Hide resolved

pitrou reviewed Jan 16, 2025

View reviewed changes

mapleFU reviewed Jan 17, 2025

View reviewed changes

fix test

420c12f

etseidl reviewed Jan 17, 2025

View reviewed changes

loose the check for unencoded_byte_array_data_bytes

f16e450

wgtmac mentioned this pull request Jan 17, 2025

Omit level histogram for some max levels without loss of precision apache/parquet-java#3123

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-45283: [C++][Parquet] Omit level histogram when max level is 0 #45285

GH-45283: [C++][Parquet] Omit level histogram when max level is 0 #45285

wgtmac commented Jan 16, 2025 •

edited by github-actions bot

Loading

github-actions bot commented Jan 16, 2025

wgtmac commented Jan 16, 2025

etseidl Jan 16, 2025

wgtmac Jan 17, 2025

etseidl Jan 16, 2025

etseidl left a comment

etseidl Jan 16, 2025

wgtmac Jan 17, 2025

etseidl Jan 17, 2025

raulcd commented Jan 16, 2025

etseidl commented Jan 16, 2025

pitrou commented Jan 16, 2025

pitrou Jan 16, 2025

wgtmac Jan 17, 2025

mapleFU left a comment

etseidl Jan 17, 2025

wgtmac Jan 17, 2025

GH-45283: [C++][Parquet] Omit level histogram when max level is 0 #45285

Are you sure you want to change the base?

GH-45283: [C++][Parquet] Omit level histogram when max level is 0 #45285

Conversation

wgtmac commented Jan 16, 2025 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jan 16, 2025

wgtmac commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etseidl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulcd commented Jan 16, 2025

etseidl commented Jan 16, 2025

pitrou commented Jan 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgtmac commented Jan 16, 2025 •

edited by github-actions bot

Loading