kosiew commented on code in PR #24770:
URL: https://github.com/apache/datafusion/pull/24770#discussion_r3912186232
##########
datafusion/datasource-parquet/src/opener/mod.rs:
##########
@@ -794,10 +800,17 @@ impl ParquetMorselizer {
// Note that if there are statistics for partition columns there will
be overlap,
// but since we use a HashMap, we'll just overwrite the partition
values with the
// constant values from statistics (which should be the same).
- literal_columns.extend(constant_columns_from_stats(
+ let stats_constants = constant_columns_from_stats(
partitioned_file.statistics.as_deref(),
&logical_file_schema,
- ));
+ );
+ let stats_constants_in_predicate = !stats_constants.is_empty()
Review Comment:
I think this guard is a little too broad. `stats_constants_in_predicate`
tells us that the predicate references some column proven constant by file
statistics, but it does not tell us that the stats substitution is what caused
the final predicate to become `false` or `NULL`.
For example, consider `a = 1 AND missing_b = 2` on a file where statistics
prove `a = 1`. This guard passes because `a` is a stats-derived constant. Stats
folding leaves that conjunct as `true`, but the missing-column adapter can then
fold `missing_b = 2` to `NULL` or `false`. We would then skip all row groups
even though the stats substitution itself did not prove the predicate
unsatisfiable.
Before this change, that case follows the intentionally preserved
missing-column path and scans/filter-prunes instead. Could we track whether the
stats substitution itself makes the predicate unsatisfiable, before
missing-column or partition rewriting, or otherwise exclude this mixed case?
It would also be good to add a regression test with one stats-constant
present column and one missing column.
##########
datafusion/datasource-parquet/src/opener/mod.rs:
##########
@@ -3023,6 +3075,100 @@ mod test {
assert_eq!(num_rows, 0);
Review Comment:
Nice to have coverage for the all-NULL case. Could we also add a small test
where exact non-NULL min/max statistics prove a column constant and `a =
<different literal>` simplifies to `false`? That would explicitly cover both
the `NULL` and `false` branches handled by the new pruning logic.
--
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]
