jayzhan211 opened a new issue, #25253: URL: https://github.com/apache/datafusion/issues/25253
### Is your feature request related to a problem or challenge? #25242 fixed wrong results by treating a missing Parquet `null_count` as unknown. That is correct in general, but parquet-rs before 53.1.0 left out `null_count` whenever it was zero ([apache/arrow-rs#6490](https://github.com/apache/arrow-rs/pull/6490), released in 53.1.0 on 2024-10-02). The 53.0.0 writer: ```rust // record null counts if greater than zero. let null_count = stats .null_count_opt() .map(|value| value as i64) .filter(|&x| x > 0); ``` For those files a missing count is exactly zero, yet DataFusion now treats it as unknown. On nullable columns with no nulls, these files lose: - sort pushdown `Exact`, so a `SortExec` comes back for pre-sorted files - `IS NULL` row-group pruning - answering `COUNT(col)` from metadata - TopK `NULLS FIRST` runtime row-group pruning This includes every file written by DataFusion < 42.1.0 and by other parquet-rs writers from before October 2024. The upgrade guide for 56.0.0 documents the regression and suggests rewriting files, but rewriting large, long-lived datasets is expensive. ### Describe the solution you'd like Decide once per file, from `FileMetaData::created_by()`, whether a missing count means zero: ```rust /// parquet-rs before 53.1.0 left out `null_count` when it was zero /// (apache/arrow-rs#6490), so for those writers a missing count is exactly /// zero. For every other writer a missing count is unknown. fn missing_null_counts_are_zero(file_metadata: &FileMetaData) -> bool { let Some((writer, version)) = file_metadata .created_by() .and_then(|s| s.split_once(" version ")) else { return false; }; let mut parts = version.split(['.', ' ', '-']).map(str::parse::<u64>); let (Some(Ok(major)), Some(Ok(minor))) = (parts.next(), parts.next()) else { return false; }; match writer { "parquet-rs" => (major, minor) < (53, 1), // DataFusion 42.1.0 was the first release to require parquet >= 53.1.0. // A 42.0.x build may have resolved a newer parquet, but those writers // never leave out a count, so a missing count still means zero. "datafusion" => (major, minor) < (42, 1), _ => false, } } ``` Pass the result to `StatisticsConverter::with_missing_null_counts_as_zero` everywhere row-group null counts are read: `RowGroupPruningStatistics` (static, runtime and fully-matched) and file statistics in `DFParquetMetadata::statistics_from_parquet_metadata`. Because the zero is exact for these files, file statistics can report `Exact` again, which brings back sort pushdown and the metadata `COUNT`. Unknown writers, custom `created_by` strings and strings that don't parse all return `false`, so this can never be less correct than #25242. When this lands, update the 56.0.0 upgrade guide entry to say these files are recognised automatically. ### Describe alternatives you've considered - A session config such as `datafusion.execution.parquet.missing_null_counts_as_zero`. It is simpler, but it applies to every file, so one file from another writer brings the wrong results back. At most it could be an opt-in for writers the check can't recognise. - Relying on the upgrade guide and file rewrites alone. ### Additional context - Tests: reuse the footer-rewrite fixture from #25242 with `WriterProperties::set_created_by`: - `parquet-rs version 53.0.0` and `datafusion version 42.0.0`: pruning, `Exact` file statistics and sort pushdown `Exact` all come back. - `parquet-rs version 53.1.0`, `parquet-mr version 1.13.1`, a custom string, and no `created_by`: stays conservative. - Known gaps (these only cost speed, never correctness): - Writers built on parquet-rs with their own `created_by` string don't get the speed back. - A tool that strips counts while keeping an old parquet-rs `created_by` would be misread. That seems very unlikely. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
