alamb commented on issue #25484:
URL: https://github.com/apache/datafusion/issues/25484#issuecomment-5735780447
Two things I found while building the reproducer that affect how this should
be implemented.
## 1. Relaxing `has_untrusted_min_max_order` is necessary but not sufficient
`StatisticsConverter` never surfaces INT96 bounds, so the pruning predicate
sees nulls even when the footer holds correct statistics. Against the file
attached above:
```
arrow ts field : Field { "ts": Timestamp(ns) }
row_group_mins : PrimitiveArray<Timestamp(ns)> [ null, null ]
row_group_maxes: PrimitiveArray<Timestamp(ns)> [ null, null ]
=> 2 of 2 row-group mins came back NULL
```
The converter's `DataType::Timestamp` arm only reads `Statistics::Int64`, so
`Statistics::Int96` falls through as null:
https://github.com/apache/arrow-rs/blob/60.0.0/parquet/src/arrow/arrow_reader/statistics.rs#L476-L484
So this needs an arrow-rs change as well, converting `Statistics::Int96`
into the timestamp type the reader produces (`Timestamp(Nanosecond)` by
default, or the unit chosen by `datafusion.execution.parquet.coerce_int96`).
Note that `Int96::to_nanos` wraps on overflow, so an out-of-range Julian day
should yield a null bound rather than a silently wrong one.
## 2. No Spark-written file can benefit from this yet
parquet-java 1.17's `PrimitiveType.requireValidColumnOrder` rejects every
`ColumnOrder` except `UNDEFINED` for INT96 ("The column order %s is not
supported by INT96"), and an undefined order means no statistics are written at
all. I confirmed this against the jars Spark 4.2 ships: a Spark-written INT96
column has no min/max whatsoever, which is why the example above is written
with parquet-rs rather than Spark.
So the immediate beneficiaries are files written by parquet-rs 60+. Spark,
Hive, Impala and Photon files follow only once parquet-java also implements
[parquet-format#584](https://github.com/apache/parquet-format/pull/584). That
does not change the case for doing this, but it is worth being clear that it
will not retroactively speed up the existing INT96 corpus.
## Note for whoever picks this up
`undefined_int96_order_is_never_trusted` in
`datafusion/datasource-parquet/src/statistics_order_tests.rs` currently asserts
the opposite of the desired behaviour, including for the
`ColumnOrder::INT96_TIMESTAMP_ORDER` case, and will need updating.
--
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]
