[EPIC] Continued correct and improved extracting Parquet statistics into ArrayRefs

[alamb]

Is your feature request related to a problem or challenge?

I consolidated the content of our previous tickets about better statistics #10806 and #10806 into a new Epic and cleaned up the subtasks

Describe the solution you'd like

Subtasks:

• DataFusion ignores "column order" parquet statistics specification #10586
• Incorrect statistics read for struct array in parquet #10609
• Reduce test duplication in tests for data page stattistics #11000
• Improve performance of DataPage statistics extraction using StringBuilder  #11281
• Incorrect statistics extracted from parquet data pages when all values are null #11280
• Update ListingTable to use StatisticsConverter #10923
• StatisticsConverter::row_group_null_counts incorrect for missing column #10926
• Support extracting Int8, Int16, Int32 statistics from Parquet Data Pages #10928
• Support extracting UInt{8, 16, 32, 64} statistics from Parquet Data Pages #10952
• Support String/LargeString and Binary/LargeBinary Parquet Data Page Statistics #11026
• Support FixedSizedBinaryArray Parquet Data Page Statistics #11184
• Support DictionaryArray Parquet Data Page Statistics #11185
• Support Boolean Parquet Data Page Statistics #11027
• Support Decimal and Decimal256 Parquet Data Page Statistics #11111
• Support Timestamp Parquet Data Page Statistics #11112
• Support Date Parquet Data Page Statistics #11113
• Support Time Parquet Data Page Statistics #11114
• Change StatisticsConverter::row_group_counts to return None for non existent columns in parquet files #10965
• Support extracting Float{16, 32, 64} statistics from Parquet Data Pages #10951
• Add a benchmark for extracting parquet data page statistics #10934
• Update the parquet code prune_pages_in_one_row_group to use the StatisticsExtractor #11480
• remove rendant tests in parquet statistics
• Port code / tests upstream: Add function that converts from parquet statistics ParquetStatistics to arrow arrays ArrayRef arrow-rs#4328

Describe alternatives you've considered

No response

Additional context

No response

[alamb]

Wow this looks like it is basically done now. 😮 Thanks to everyone who helped

#10609 still needs a look from @Lordworms

I think the final piece of work is to port the code + tests upstream to arrow-rs

[Lordworms]

Wow this looks like it is basically done now. 😮 Thanks to everyone who helped

#10609 still needs a look from @Lordworms

I think the final piece of work is to port the code + tests upstream to arrow-rs

I'll take a look then, I forget about this one.... 😅

[efredine]

I think I've learnt enough that I can probably manage the upstream port to arrow-rs @alamb

There are probably a few performance improvements we might be able to make (using StringBuilder for example) and it's perhaps worth revisiting whether the tests can/should be restructured as per #11000?

[efredine]

Further to the performance discussion @alamb - the StringBuilder pattern you suggested in #11136 (comment) does seem to materially improve performance:

Extract data page statistics for String/extract_statistics/String
                        time:   [15.368 µs 15.405 µs 15.446 µs]
                        change: [-68.672% -68.540% -68.409%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  4 (4.00%) high mild

So seems like a worthwhile thing to go ahead with? I think there are several places where we can do something similar.

One question - I notice in that ticket that you 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.

[efredine]

@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

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?

(It's entirely possible I'm misunderstanding something here, if so, apologies in advance!)

[alamb]

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?

I think you are correct -- that is a very insightful conclusion @efredine

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.

Is this something you are willing to do? I filed #11280 to track

[alamb]

So seems like a worthwhile thing to go ahead with? I think there are several places where we can do something similar.

I agree @efredine -- thank you -- I filed #11281 to track

[alamb]

Update here is @efredine has a PR up for porting this upstream: apache/arrow-rs#6046 ❤

If no one beats me to it I plan to review that PR this and then make a draft PR to use the upstream implementation in DataFusion when it is available, and then we can close this issue. Very exciting

[alamb]

Closing this epic as I think it is basically complete. The final piece #11479 is waiting on the next arrow-rs release apache/arrow-rs#5998) but I don't think there is any reason to leave this open