parquet Statistics - deprecate `has_*` APIs and add `_opt` functions that return `Option<T>` #6216

Michael-J-Ward · 2024-08-09T08:24:58Z

Which issue does this PR close?

Closes #6093
Closes #6215

Rationale for this change

From the feature request for min and max

The fact that Parquet metadata statistics requires checking before access is > confusing and error prone, for example #6092

From the feature request for null_count

and the has_nulls() is based on null_count > 0, leading to ambiguity when null_count equals 0: either all values are non-null, or just that the null count stat is missing.

What changes are included in this PR?

APIs added

Statistics::{min,max}_bytes_opt
ValueStatistics::{min,max}_bytes_opt
ValueStatistics::{min,max}_opt
Statistics::null_count_opt
ValueStatistics::null_count_opt

APIs deprecated

Statistics::has_nulls
Statistics::null_count
ValueStatistics::null_count
Statistics::has_min_max_set
ValueStatistic::has_min_max_set
Statistics::{min,max}_bytes
ValueStatistics::{min,max}_bytes
ValueStatistics::{min,max}

Are there any user-facing changes?

Yes. All of the above changes are changes to the public APi.

Additionally, null_count = 0 is now written to page statistics instead of being treated as None.

I first re-named the existing method to `min_unchecked` and made it internal to the crate. I then added a `pub min(&self) -> Opiton<&T>` method. I figure we can first change the public API before deciding what to do about internal usage. Ref: apache#6093

I first re-named the existing method to `max_unchecked` and made it internal to the crate. I then added a `pub max(&self) -> Opiton<&T>` method. I figure we can first change the public API before deciding what to do about internal usage. Ref: apache#6093

Ref: apache#6093

This removes ambiguity around whether the between all values are non-null or just that the null count stat is missing Ref: apache#6215

Changing null_count from u64 to Option<u64> increases the memory size and layout of the metadata. I included these tests as a separate commit to call extra attention to it.

Michael-J-Ward

Lastly, I'd like to call attention to the change in memory size by switching from null_count: u64 to null_count: Option<u64>.

I updated the memory size and layout tests to pass, but I'm unsure if those values were "sacred".

parquet/src/file/statistics.rs

parquet/src/arrow/arrow_reader/statistics.rs

parquet/src/column/writer/mod.rs

parquet/src/file/statistics.rs

crepererum

I've also ran into this weird API before (which I think is inspired by certain C++ interfaces) and I think this new API is better and more "Rusty".

crepererum · 2024-08-12T12:13:48Z

I'm wondering if we need an API deprecation here or not. If we need one, we would need to use an approach like this:

has_... and min/max/....: add deprecation note
add new Option APIs: min_opt/max_opt/...

That would be the smoothest path. However, the API breakage is rather simple and easy to fix. @alamb WDYT?

Michael-J-Ward · 2024-08-13T16:39:15Z

@crepererum - I'm happy to go either way, just let me know.

I went with the breaking change because I thought removing such a foot-gun is suitable for a breaking-change / major-release upgrade. Also, that's what the GH issues requested.

alamb

Thank you @Michael-J-Ward this is epic. Thank you @crepererum for the review

In my opinion we should make all the APIs consistent (aka change max_bytes to return Option<&[u8]> etc

Responding to @crepererum 's comment in #6216 (comment)

has_... and min/max/....: add deprecation note
add new Option APIs: min_opt/max_opt/...
That would be the smoothest path. However, the API breakage is rather simple and easy to fix. @alamb WDYT?

I would also be ok with either path as long as the API is consistent (e.g. min_bytes should do the same thing as min)

I would personally prefer erring on the "nicer experience for users of this crate" and thus go the backwards compatible route:

Leave the existing functions as is but mark them deprecated
Add new functions like min_opt(), max_opt(), etc that return Option<..>

That means users will be told what is going on with the deprecated APIs and how to fix it, even though the eventual API may not be as concise.

alamb · 2024-08-13T20:41:26Z

parquet/tests/arrow_writer_layout.rs

@@ -189,7 +189,7 @@ fn test_primitive() {
                    pages: (0..8)
                        .map(|_| Page {
                            rows: 250,
-                            page_header_size: 36,
+                            page_header_size: 38,


why are these changes needed?

These sizes changed upon switching ValueStatistic::null_count from u64 to Option<u64>.

Although I'd expect such a change to require an extra bit, I was still concerned that these sizes might be set by the parquet spec, and so called it out.

If these sizes are sacred and can't be updated, I'd appreciate any pointers for implementing null_count_opt without affecting them.

#6216 (review)

Scratch the above. The answer is in the linked comment.

When I initially converted null_count to Option<64>, the first test I updated test_memory_size, and incorrectly assume the rest of the layout tests were downstream of that one.

#6216 (comment)

parquet/src/file/statistics.rs

parquet/src/column/writer/mod.rs

parquet/src/file/statistics.rs

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Per PR review, we will deprecate the old API instead of introducing a brekaing change. Ref: apache#6216 (review)

This adds the API and migrates all of the test usage. The old APIs will be deprecated next.

The check is unnecessary now that the stats funcs return Option<T> when unset.

An internal version was also created because it is used so extensively in testing.

…s_nulls

Michael-J-Ward · 2024-08-14T13:57:49Z

parquet/src/file/statistics.rs

-        } else {
-            None
-        },
+        null_count: stats.null_count_opt().map(|value| value as i64),


@alamb - this is why the arrow_writer_layout tests changed.

The previous API treated null_count = 0 as None.

The new API treats null_count = 0 as Some(0).

I believe the new behavior is what is desired, but can easily revert to the old behavior with:

null_count: stats.null_count_opt().map(|value| value as i64).filter(|&x| x > 0),

Update -- I think this code is related to reading the statistics that were in the parquet file rather than how they are written.

It seems like previously this code just set the statistics count to zero #6256 tracks this

I agree the new behavior is desired, but I think it changes what values are written to parquet files (specifically the parquet metadata will now have the thrift equivalent of Some(0) rather than the equivalent of None. I filed #6256 to track

As this PR is already quite large, I think we should split it into two parts:

The API changes

The change for writing the metadata

I plan to update this PR to revert the changes to the metadata writing, and will then make a follow on PR to discuss / propose changing the statistics that are written to the file

This removes the assertion from any test that subsequently unwraps both min_opt and max_opt.

…th assertions on min_opt and max_opt This removes all use of Statistics::_internal_has_min_max_set from the code base, and so it is also removed.

Michael-J-Ward · 2024-08-14T15:16:47Z

@alamb and @crepererum - this PR is ready for another look.

alamb

Thank you so much for this PR @Michael-J-Ward -- this improvement is a very long time coming.

I plan to split it into two parts -- I'll leave this PR with the API change and make a new PR with the change to the files that are written

alamb · 2024-08-15T15:53:05Z

I pushed d4e650 to revert the writer behavior changes and added some comments -- let me know what you think

alamb · 2024-08-15T17:00:35Z

I am going to merge this PR as the APIs code has been reviewed and the content of what is read/written to statistics is the same as on master. I will open follow on PRs for discussion and am happy to make other changes if desired

alamb

Thanks again @Michael-J-Ward -- epic

alamb · 2024-08-15T19:35:34Z

A small follow up from this PR #6259

Michael-J-Ward added 3 commits August 9, 2024 03:02

cargo fmt

7ffac1a

github-actions bot added the parquet Changes to the parquet crate label Aug 9, 2024

Michael-J-Ward added 3 commits August 9, 2024 03:40

remove Statistics::has_min_max_set from the public api

87c375a

Ref: apache#6093

update impl HeapSize for ValueStatistics to use new min and max api

6950994

migrate all tests to new Statistics min and max api

c65a773

Michael-J-Ward force-pushed the parquet-has-min-max branch from 2b21daf to c65a773 Compare August 9, 2024 08:47

alamb mentioned this pull request Aug 9, 2024

Parquet Statistics null_count does not distinguish between 0 and not specified #6215

Closed

Michael-J-Ward added 2 commits August 9, 2024 14:22

make Statistics::null_count return Option<u64>

89ea21d

This removes ambiguity around whether the between all values are non-null or just that the null count stat is missing Ref: apache#6215

update expected metadata memory size tests

f2785f8

Changing null_count from u64 to Option<u64> increases the memory size and layout of the metadata. I included these tests as a separate commit to call extra attention to it.

Michael-J-Ward changed the title ~~parquet Statistics - remove has_min_max_set and return Option<T> for min and max~~ parquet Statistics - remove has_* APIs and return Option<T> for statistics Aug 9, 2024

add TODO question on is_min_max_backwards_compatible

9a4921f

Michael-J-Ward commented Aug 9, 2024

View reviewed changes

crepererum approved these changes Aug 12, 2024

View reviewed changes

alamb reviewed Aug 13, 2024

View reviewed changes

Michael-J-Ward and others added 11 commits August 13, 2024 17:14

Apply suggestions from code review

7e082a7

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

update ValueStatistics::max docs

ddd8498

rename new optional ValueStatistics::max to max_opt

e92321b

Per PR review, we will deprecate the old API instead of introducing a brekaing change. Ref: apache#6216 (review)

rename new optional ValueStatistics::min to min_opt

e4db777

add Statistics:{min,max}_bytes_opt

bd83a38

This adds the API and migrates all of the test usage. The old APIs will be deprecated next.

update make_stats_iterator macro to use *_opt methods

4e2909b

deprecate non *_opt Statistics and ValueStatistics methods

2f3e21e

remove stale TODO comments

050ba89

remove has_min_max_set check from make_decimal_stats_iterator

fc0ed20

The check is unnecessary now that the stats funcs return Option<T> when unset.

deprecate has_min_max_set

2366942

An internal version was also created because it is used so extensively in testing.

switch to null_count_opt and reintroduce deprecated null_count and ha…

82b40b8

…s_nulls

Michael-J-Ward commented Aug 14, 2024

View reviewed changes

Michael-J-Ward added 2 commits August 14, 2024 09:43

remove redundant test assertions of stats._internal_has_min_max_set

3efc631

This removes the assertion from any test that subsequently unwraps both min_opt and max_opt.

replace negated test assertions of stats._internal_has_mix_max_set wi…

a23cf8b

…th assertions on min_opt and max_opt This removes all use of Statistics::_internal_has_min_max_set from the code base, and so it is also removed.

crepererum approved these changes Aug 15, 2024

View reviewed changes

alamb mentioned this pull request Aug 15, 2024

parquet writer does not encode null count = 0 correctly #6256

Open

alamb reviewed Aug 15, 2024

View reviewed changes

alamb added the api-change Changes to the arrow API label Aug 15, 2024

alamb changed the title ~~parquet Statistics - remove has_* APIs and return Option<T> for statistics~~ parquet Statistics - deprecate has_* APIs and add _opt functions that return Option<T> for statistics Aug 15, 2024

alamb changed the title ~~parquet Statistics - deprecate has_* APIs and add _opt functions that return Option<T> for statistics~~ parquet Statistics - deprecate has_* APIs and add _opt functions that return Option<T> Aug 15, 2024

alamb added 2 commits August 15, 2024 11:52

Revert changes to parquet writing, update comments

7d4e650

Merge remote-tracking branch 'apache/master' into parquet-has-min-max

48a35d3

alamb approved these changes Aug 15, 2024

View reviewed changes

alamb merged commit 69b17ad into apache:master Aug 15, 2024
16 checks passed

alamb mentioned this pull request Aug 15, 2024

Add Statistics::distinct_count_opt and deprecate Statistics::distinct_count #6259

Merged

alamb mentioned this pull request Aug 16, 2024

Update to arrow/parquet 53.0.0, tonic, prost, object_store, pyo3 apache/datafusion#12032

Merged

Michael-J-Ward mentioned this pull request Aug 16, 2024

Write null counts in parquet files when they are present #6257

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parquet Statistics - deprecate `has_*` APIs and add `_opt` functions that return `Option<T>` #6216

parquet Statistics - deprecate `has_*` APIs and add `_opt` functions that return `Option<T>` #6216

Michael-J-Ward commented Aug 9, 2024 •

edited

Loading

Michael-J-Ward left a comment

crepererum left a comment •

edited

Loading

crepererum commented Aug 12, 2024

Michael-J-Ward commented Aug 13, 2024

alamb left a comment

alamb Aug 13, 2024

Michael-J-Ward Aug 14, 2024 •

edited

Loading

Michael-J-Ward Aug 14, 2024

Michael-J-Ward Aug 14, 2024

alamb Aug 15, 2024

alamb Aug 15, 2024

Michael-J-Ward commented Aug 14, 2024

alamb left a comment

alamb commented Aug 15, 2024 •

edited

Loading

alamb commented Aug 15, 2024

alamb left a comment

alamb commented Aug 15, 2024

parquet Statistics - deprecate has_* APIs and add _opt functions that return Option<T> #6216

parquet Statistics - deprecate has_* APIs and add _opt functions that return Option<T> #6216

Conversation

Michael-J-Ward commented Aug 9, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

APIs added

APIs deprecated

Are there any user-facing changes?

Michael-J-Ward left a comment

Choose a reason for hiding this comment

crepererum left a comment • edited Loading

Choose a reason for hiding this comment

crepererum commented Aug 12, 2024

Michael-J-Ward commented Aug 13, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Aug 13, 2024

Choose a reason for hiding this comment

Michael-J-Ward Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Michael-J-Ward Aug 14, 2024

Choose a reason for hiding this comment

Michael-J-Ward Aug 14, 2024

Choose a reason for hiding this comment

alamb Aug 15, 2024

Choose a reason for hiding this comment

alamb Aug 15, 2024

Choose a reason for hiding this comment

Michael-J-Ward commented Aug 14, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb commented Aug 15, 2024 • edited Loading

alamb commented Aug 15, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb commented Aug 15, 2024

parquet Statistics - deprecate `has_*` APIs and add `_opt` functions that return `Option<T>` #6216

parquet Statistics - deprecate `has_*` APIs and add `_opt` functions that return `Option<T>` #6216

Michael-J-Ward commented Aug 9, 2024 •

edited

Loading

crepererum left a comment •

edited

Loading

Michael-J-Ward Aug 14, 2024 •

edited

Loading

alamb commented Aug 15, 2024 •

edited

Loading