Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect statistics extracted from parquet data pages when all values are null #11280

Closed
Tracked by #10922
alamb opened this issue Jul 5, 2024 · 3 comments · Fixed by #11295
Closed
Tracked by #10922

Incorrect statistics extracted from parquet data pages when all values are null #11280

alamb opened this issue Jul 5, 2024 · 3 comments · Fixed by #11295
Assignees

Comments

@alamb
Copy link
Contributor

alamb commented Jul 5, 2024

Originally posted by @efredine in #10922 (comment)

We always flatten the date page stats iterator - following the pattern from the initial PR: https://github.com/apache/datafusion/pull/10852/files#diff-7110f4709c105a18ef74a212396444d62052179a735d148fb62470a8b157fb40R582

But I'm wondering if flatten is the right thing to do here?

The min or max values for each page will be None if all the values on the page happen to be null: https://github.com/apache/arrow-rs/blob/master/parquet/src/file/page_index/index.rs#L37-L44

Using flatten in this case will mean that the length of result for that page will be shorter than the number of data pages? So, is it possible that rather than flatten we instead want to do something like a flat map where the Some values are flattened and None values are mapped to a null value?

Potential user impact:

The code appended nulls for missing values. However, I think in most cases, missing values are simply omitted because all the None values are removed by flattening. So, in general, users of the data page statistics will need to check whether or not the length of the array matches the number of actual data pages? This is different from how the row group statistics are handled - they will instead have a null value for any missing statistics.

Is this difference in behaviour expected or just a side effect of the implementation.

A: I think it is a side effect of implementation and not a good one

@alamb
Copy link
Contributor Author

alamb commented Jul 5, 2024

Ideally what I think we should do is to write up a test case (using your suggestion of a column / page that is entirely null) and verify there is a problem / fix it.

@alamb alamb changed the title @marvinlanhenke @alamb We always flatten the date page stats iterator - following the pattern from the initial PR: https://github.com/apache/datafusion/pull/10852/files#diff-7110f4709c105a18ef74a212396444d62052179a735d148fb62470a8b157fb40R582 Incorrect statistics extracted from parquet data pages when all values are null Jul 5, 2024
@efredine
Copy link
Contributor

efredine commented Jul 5, 2024

Take

@efredine
Copy link
Contributor

efredine commented Jul 5, 2024

Ok - quick update - I had some misunderstanding of what was going on. I think there may still be a problem, but its different from what I originally thought.

My misunderstanding: the top level flatten is because we have an iterator of iterators. The inner iterator is iterating over the page indexes within a ColumnIndex. That inner iterator returns an Option.

And the original PR added an explicit test case for the scenario of a data page with all null values:
https://github.com/apache/datafusion/blob/main/datafusion/core/tests/parquet/arrow_statistics.rs#L475-L504

However, the tests for all the other data types don't cover this scenario and there are a bunch of places where we are doing a filter_map in the inner loop which should probably just be a map. But I need to write tests to prove this first and then expand the coverage if it turns out to be the case.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants