Fix several bugs in parquet writer statistics generation, add `EnabledStatistics` to control level of statistics generated #2022

tustvold · 2022-07-07T12:59:24Z

Which issue does this PR close?

Closes #2015
Closes #2016

Rationale for this change

Fixes a number of bugs

What changes are included in this PR?

Removes max_statistics_size as it isn't used, and changes statistics to be enabled based on an enumeration. This allows consistently only producing chunk level statistics, potentially precomputed, or not.

Are there any user-facing changes?

No

tustvold · 2022-07-07T13:00:22Z

parquet/src/file/properties.rs

        self
    }
 }

+/// Controls the level of statistics to be computed by the writer
+#[derive(Debug, Clone, Copy, Eq, PartialEq)]
+pub enum EnabledStatistics {


I'm not a massive fan of this, but it was the only way I could think of to preserve the existing behaviour whilst also fixing the related bugs

I actually like this API -- as for some use cases page level statistics are likely to have a higher overhead in storage and writing than benefit (e.g. large string columns for small row groups)

It would be good to clarify in here if Page also computes Chunk level statistics.

If Page does not also include Chunk that seems strange to me (as I would expect lower level stats to also have coarser grained statistics).

Glad one of us likes it 😆

FWIW at least in the case of the column index, parquet allows using truncated values in statistics. e.g. instead of a min of "lorem ipsum ..." you could just store "lorem"

tustvold · 2022-07-07T16:58:28Z

parquet/src/column/writer.rs

-        if let Some(distinct) = distinct_count {
-            self.column_distinct_count =
-                Some(self.column_distinct_count.unwrap_or(0) + distinct);
+        // We can only set the distinct count if there are no other writes


This is the fix for #2016

tustvold · 2022-07-07T16:58:52Z

parquet/src/column/writer.rs

-            Some(self.make_page_statistics())
-        } else {
-            None
+        let has_min_max = self.min_page_value.is_some() && self.max_page_value.is_some();


This logic is moved from flush_data_pages

codecov-commenter · 2022-07-07T17:57:38Z

Codecov Report

Merging #2022 (428f0d5) into master (38370cb) will decrease coverage by 0.16%.
The diff coverage is 66.40%.

❗ Current head 428f0d5 differs from pull request most recent head 3049111. Consider uploading reports for the commit 3049111 to get more accurate results

@@            Coverage Diff             @@
##           master    #2022      +/-   ##
==========================================
- Coverage   83.59%   83.43%   -0.17%     
==========================================
  Files         222      222              
  Lines       57529    57911     +382     
==========================================
+ Hits        48091    48317     +226     
- Misses       9438     9594     +156

Impacted Files	Coverage Δ
parquet/src/arrow/array_reader/byte_array.rs	`84.47% <0.00%> (-1.24%)`	⬇️
...et/src/arrow/array_reader/byte_array_dictionary.rs	`82.26% <0.00%> (-1.66%)`	⬇️
...uet/src/arrow/array_reader/complex_object_array.rs	`93.20% <0.00%> (-1.07%)`	⬇️
parquet/src/arrow/array_reader/empty_array.rs	`45.45% <0.00%> (-10.11%)`	⬇️
parquet/src/arrow/array_reader/list_array.rs	`92.69% <0.00%> (-0.72%)`	⬇️
parquet/src/arrow/array_reader/map_array.rs	`58.82% <0.00%> (-8.98%)`	⬇️
parquet/src/arrow/array_reader/mod.rs	`88.23% <ø> (ø)`
parquet/src/arrow/array_reader/null_array.rs	`81.48% <0.00%> (-6.52%)`	⬇️
parquet/src/arrow/array_reader/primitive_array.rs	`88.63% <0.00%> (-1.02%)`	⬇️
parquet/src/arrow/array_reader/struct_array.rs	`78.99% <0.00%> (-9.69%)`	⬇️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38370cb...3049111. Read the comment docs.

alamb

I had some comments on the API design and deprecation, but the actual code looks good and like a significant improvement to me. 👍

alamb · 2022-07-07T21:43:57Z

parquet/src/arrow/schema.rs

@@ -152,7 +152,10 @@ pub(crate) fn add_encoded_arrow_schema_to_metadata(
        value: Some(encoded),
    };

-    let mut meta = props.key_value_metadata.clone().unwrap_or_default();
+    let meta = props


Does this change have any effect (other than to make the code more readable)?

There is no user-facing change, but it does avoid an unnecessary clone

alamb · 2022-07-07T21:47:19Z

parquet/src/file/properties.rs

        self
    }
 }

+/// Controls the level of statistics to be computed by the writer
+#[derive(Debug, Clone, Copy, Eq, PartialEq)]
+pub enum EnabledStatistics {


I actually like this API -- as for some use cases page level statistics are likely to have a higher overhead in storage and writing than benefit (e.g. large string columns for small row groups)

alamb · 2022-07-07T21:48:51Z

parquet/src/column/writer.rs

-                    .map_or(true, |v| self.compare_greater(v, min))
-                {
-                    self.min_column_value = Some(min.clone());
+        if self.statistics_enabled == EnabledStatistics::Chunk {


I would personally find match self.statistics_enabled ... to be easier to validate that other variants of EnabledStatistics::Chunk did not apply.

Or perhaps a function EnabledStatistics::compute_chunk_statistics().

Basically I am thinking about "what if someone adds a new variant" -- it would be nice if the compiler told them where to update

In fact, no that I re-read this, shouldn't this also be set if we are computing page level statistics?

I'll add a comment explaining why we only care if it is exactly this variant, any other variants would need to be computed at a different level

alamb · 2022-07-07T21:50:58Z

parquet/src/column/writer.rs

-            (None, None) => {}
+                (None, None) => {
+                    for val in values {
+                        Self::update_min(&self.descr, val, &mut self.min_column_value);


Is this the fix for #2015?

alamb · 2022-07-07T21:52:19Z

parquet/src/column/writer.rs

+    /// Writer may optionally provide pre-calculated statistics for this batch
+    ///
+    /// Note: Unless disabled using by using [`WriterProperties`] to set
+    /// enabled statistics to [`EnabledStatistics::Chunk`], this will still compute


This comment almost reads the opposite of what I think the code is doing (using the passed in pre-calculated statistics or computing them if passed in)

alamb · 2022-07-07T21:54:44Z

parquet/src/file/properties.rs

        self
    }
 }

+/// Controls the level of statistics to be computed by the writer
+#[derive(Debug, Clone, Copy, Eq, PartialEq)]
+pub enum EnabledStatistics {


It would be good to clarify in here if Page also computes Chunk level statistics.

If Page does not also include Chunk that seems strange to me (as I would expect lower level stats to also have coarser grained statistics).

alamb · 2022-07-07T21:56:24Z

parquet/src/file/properties.rs

        self.statistics_enabled = Some(enabled);
    }

-    /// Sets max size for statistics for this column.


I don't see any mention of max_statistics_size in this PR description or the linked tickets.

I think it is a reasonable feature (to limit bloating parquet files). Is the issue that it was ignored? If so perhaps we can leave the API in with a warn or deprecate that it is ignored and file a ticket to properly support it?

I will back out this change

alamb · 2022-07-07T21:57:11Z

parquet/src/column/writer.rs

+    /// Perform a conditional update of `cur`, skipping any NaN values
+    ///
+    /// If `cur` is `None`, sets `cur` to `Some(val)`, otherwise calls `should_update` with
+    /// the value of `cur`, and updates `cur` to `Some(val)` if it returns `true`


cc @crepererum as I think you added the initial NaN handling a while ago

If I read this correctly the NaN handling here is to avoid "poisoning" which is OK.

IMHO the stats should include a flag if there were any NaN values, but this is far outside the scope of this PR since this would affect the parquet format as a whole.

alamb · 2022-07-07T21:58:37Z

parquet/src/column/writer.rs

+        assert_eq!(pages[1].page_type(), PageType::DATA_PAGE);
+
+        let page_statistics = pages[1].statistics().unwrap();
+        assert_eq!(page_statistics.min_bytes(), 1_i32.to_le_bytes());


tustvold added the api-change Changes to the arrow API label Jul 7, 2022

github-actions bot added the parquet Changes to the parquet crate label Jul 7, 2022

tustvold commented Jul 7, 2022

View reviewed changes

Fix parquet writer statistics

06cfa35

tustvold force-pushed the fix-parquet-writer-statistics branch from 034be68 to 06cfa35 Compare July 7, 2022 13:07

tustvold added 2 commits July 7, 2022 09:21

Fix test_column_writer_precalculated_statistics

a9f7484

Handle NaN floats

e251366

tustvold commented Jul 7, 2022

View reviewed changes

Reduce code duplication

3049111

alamb changed the title ~~Fix parquet writer statistics~~ Fix several bugs in parquet writer statistics generation Jul 7, 2022

alamb changed the title ~~Fix several bugs in parquet writer statistics generation~~ Fix several bugs in parquet writer statistics generation, add EnabledStatistics to control level of statistics generated Jul 7, 2022

alamb approved these changes Jul 7, 2022

View reviewed changes

Review feedback

d7a4a89

tustvold merged commit d9dbf72 into apache:master Jul 8, 2022

This was referenced Jul 8, 2022

Parquet Writer Ignores Statistics specification in WriterProperties #2014

Closed

Parquet Writer Ignores "max statistics size" specification in WriterProperties #2033

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix several bugs in parquet writer statistics generation, add `EnabledStatistics` to control level of statistics generated #2022

Fix several bugs in parquet writer statistics generation, add `EnabledStatistics` to control level of statistics generated #2022

tustvold commented Jul 7, 2022 •

edited

tustvold Jul 7, 2022

alamb Jul 7, 2022

alamb Jul 7, 2022

tustvold Jul 8, 2022 •

edited

tustvold Jul 7, 2022

tustvold Jul 7, 2022

codecov-commenter commented Jul 7, 2022

alamb left a comment

alamb Jul 7, 2022

tustvold Jul 8, 2022 •

edited

alamb Jul 7, 2022

alamb Jul 7, 2022

alamb Jul 7, 2022

tustvold Jul 8, 2022

alamb Jul 7, 2022

tustvold Jul 8, 2022

alamb Jul 7, 2022

alamb Jul 7, 2022

alamb Jul 7, 2022

tustvold Jul 8, 2022

alamb Jul 7, 2022

crepererum Jul 8, 2022

alamb Jul 7, 2022

Fix several bugs in parquet writer statistics generation, add EnabledStatistics to control level of statistics generated #2022

Fix several bugs in parquet writer statistics generation, add EnabledStatistics to control level of statistics generated #2022

Conversation

tustvold commented Jul 7, 2022 • edited

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Jul 8, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 7, 2022

Codecov Report

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Jul 8, 2022 • edited

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix several bugs in parquet writer statistics generation, add `EnabledStatistics` to control level of statistics generated #2022

Fix several bugs in parquet writer statistics generation, add `EnabledStatistics` to control level of statistics generated #2022

tustvold commented Jul 7, 2022 •

edited

tustvold Jul 8, 2022 •

edited

tustvold Jul 8, 2022 •

edited