JingsongLi commented on PR #9114:
URL: https://github.com/apache/paimon/pull/9114#issuecomment-5230130334
Thanks for working on this. I compared the new Python group-stats pruning
path with the Java data-evolution implementation. There are correctness gaps on
both sides that should be addressed in this PR before this optimization is
enabled.
### Python changes
1. **Rebind projected predicates to the full table schema before group-stats
evaluation.**
`ReadBuilder.new_predicate_builder()` builds against `read_type()`, so
after a projection the predicate index is projection-relative.
`DataEvolutionGroupStatsFilter`, however, interprets that index against
`self.table.fields`.
A concrete reproducer is a table `[id, b, c]`, projection `[id, c]`, and
predicate `c > 150`. The predicate carries index 1, so the new scanner tests
the stats for `b` and can prune a group that contains `c = 200`. Please
rewrite/rebind the stats predicate by field name to `table.fields`, as the
reader path already does, and add a regression test.
2. **Do not use finite-only PyArrow min/max to prove negative FLOAT/DOUBLE
predicates.**
Python/PyArrow may record `[5.0, NaN]` as `min = max = 5.0`. The new
group filter then prunes `x != 5.0`, although the NaN row matches at row level.
The Java collector does not have the same behavior because Java floating-point
comparison keeps NaN in the bounds. Please either make Python stats NaN-aware
or fail open for negative floating-point predicates when the metadata cannot
prove that NaN is absent. Please add `notEqual`/`notIn` regression coverage.
3. **Treat only a genuinely missing field as exact NULL.**
For a known file field, `null_count == row_count` together with non-null
min/max is contradictory metadata, not proof of an all-NULL column. Please
validate this cross-invariant and fail open. `_MISSING` may use exact-NULL
evaluation; valid `_KNOWN` stats should go through the normal simple-stats
predicate path.
4. **Align provider lookup with Java's cached offset approach.**
The current Python implementation scans all files for every table field
and evolves each selected partial layout into a full-width stats vector. Please
use a cached projected layout/provider index and raw `row_offset +
field_offset` lookup (or an equivalent lazy view), instead of repeatedly
constructing full-width evolved stats.
### Java changes
Please also harden the corresponding Java implementation rather than leaving
the two implementations with different safety rules:
1. In `DataEvolutionFileStoreScan.evolutionStats`, if the newest provider
for a field has an incompatible type, mark the field stats as unknown. Do not
continue and select an older type-compatible provider, because the older stats
do not describe the latest value.
2. Groups are produced by merging overlapping row-id ranges. A provider
whose range does not cover the complete logical group cannot describe the
group's min/max/null count. Please aggregate all required ranges or
conservatively mark that field as unknown.
3. If multiple providers for a field share the latest sequence number, do
not select one based on manifest/list order. Merge them when that is provably
valid; otherwise mark the field stats as unknown.
4. In the common Java stats predicate path, fail open for invalid or
contradictory metadata: invalid null counts, only one of min/max being null,
`min > max`, or `nullCount == rowCount` with non-null bounds.
Please add focused Java and Python regression tests for these cases. The
projection-index and NaN cases are false-negative pruning bugs: the pre-change
data-evolution scan reads the matching rows, while the new Python group-stats
path drops the entire logical group.
--
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]
