Repository navigation
fix: do not treat floating point columns with equal min/max statistics as constant - #27283
compiler-segfault wants to merge 1 commit into
Conversation
github-actions
Bot
added
core
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn nan_rows_are_not_replaced_by_the_statistics_constant() { |
There was a problem hiding this comment.
I think its generally preferred to use SLT for this sort of tests.
There was a problem hiding this comment.
Thanks, moved it to a sqllogictest: datafusion/sqllogictest/test_files/parquet_constant_columns.slt (5f0bfe2), and rebased the branch on current main.
…s as constant Parquet writers exclude `NaN` from floating point `min`/`max` statistics, as the format requires, so a column holding `1.0`, `NaN` and `1.0` has `min == max == 1.0` and `null_count == 0`. `constant_value_from_stats` read that as proof that the column is constant and replaced it with the statistic value, silently turning the `NaN` row into a `1.0` row. `-0.0` and `0.0` compare equal in the same way, so the substituted value also loses the sign. Skip statistics-based constant substitution for floating point columns (including dictionary encoded ones); exact types keep the optimization. The regression test is a sqllogictest, as suggested in review.
3ef0df8 to
5f0bfe2
Compare
github-actions
Bot
added
sqllogictest
|
Fork CI cannot be dispatched at the moment: GitHub returns Actions has been disabled for this repository for the fork, so Rust (fork) shows as skipped rather than running. Verified locally on top of main at 34eb6dc: cargo test --test sqllogictests -- parquet_constant_columns is green (without the change the scan returns 1, 1, 1 instead of 1, 1, NaN), and cargo fmt --all -- --check plus cargo clippy -p datafusion-datasource-parquet --all-targets -- -D warnings are clean. |
SELECTof a Parquet float column returns wrong values: NaN rows are replaced by the statistics constant when min == max #26490.Rationale for this change
A plain
SELECTof a ParquetFloat32/Float64column can return values that are not in the file: rows holdingNaNcome back as the statistics constant.Parquet statistics exclude
NaNfrommin/max, so a chunk holding1.0, NaN, 1.0reportsmin == max == 1.0withnull_count == 0. The skip-scan path indatafusion/datasource-parquettreatsmin == maxas proof that the column is constant and substitutes the statistic for the data, so theNaNrow is silently rewritten as1.0.-0.0and0.0are affected the same way, because they compare equal.COPY ... TOfrom such a table writes the wrong values too.What changes are included in this PR?
constant_value_from_statsno longer derives a constant frommin == maxwhen the column type (or a dictionary's value type) is floating point. The all-null case is unchanged.datafusion/sqllogictest/test_files/parquet_constant_columns.slt, which writes a Parquet file with1.0, NaN, 1.0and reads it back. The Rust integration test from the first revision is replaced by it, as suggested in review.min == maxstill proves constancy for exact types, so the shortcut is only given up where it is not a proof.What is the testing strategy for this PR?
1, 1, 1, so theNaNrow is missing) and passes after it (1, 1, NaN):cargo test --test sqllogictests -- parquet_constant_columns.cargo fmt --all -- --checkandcargo clippy -p datafusion-datasource-parquet --all-targets -- -D warningsare green.Are there any user-facing changes?
No API change. Floating point columns whose statistics happen to have
min == maxare now read from the file instead of being replaced by a constant; that costs a read of those chunks and returns the correct values.AI Disclosure
This change was prepared with an AI coding assistant; I reproduced the bug locally and reviewed the diff end to end, and can answer questions about it.
Open question I would flag for review: the alternative is to plumb Parquet's
nan_countintoColumnStatisticsand keep the shortcut for float columns that reportnan_count == 0. That is a public API change, and files written by older writers do not carry the count, so this PR takes the conservative route instead.