Repository navigation
Conversation
A partition column promoted int->long (or float->double) keeps its originally-written values in manifests produced before the promotion. The entry partition tuple is decoded under the manifest's own narrower type, while the scan predicate is bound to the current wider type. `Datum::partial_cmp`/`PartialEq` require matching types, so the `ExpressionEvaluator` compared e.g. an int partition value against a long predicate literal, got no ordering, and silently pruned the data file, dropping matching rows from filtered scans. The manifest-list `ManifestEvaluator` is unaffected: it decodes `FieldSummary` bounds with `Datum::try_from_bytes`, which already widens a 4-byte int bound to long. Mirror that on the entry path by widening the partition value to the reference's type before comparing. Decimals already compare across precisions, so only int->long and float->double need widening. Co-authored-by: Isaac <no-reply@databricks.com>
Companion to the partition-tuple widening: InclusiveMetricsEvaluator compares a data file's column bounds against the scan predicate, but those bounds are decoded under the manifest's embedded (narrower) schema while the predicate is bound to the current (wider) type. For an int column promoted to long (or float to double), the inequality and IN checks compared mismatched types, got no ordering, and discarded files that could match, dropping rows; the eq check failed open and merely skipped valid pruning. Widen each stored bound to the reference's type before comparing, via the same `Datum::promote_to` helper, in the inequality, eq, and IN paths.
| /// Returns a copy whose literal is widened to `target` for the value | ||
| /// promotions `int` -> `long` and `float` -> `double`, so a value written | ||
| /// under the narrower type compares against a predicate bound to the wider | ||
| /// one. Any other `target` returns an unchanged copy; decimals already | ||
| /// compare across precisions. | ||
| pub(crate) fn promote_to(&self, target: &PrimitiveType) -> Datum { | ||
| match (&self.literal, target) { | ||
| (PrimitiveLiteral::Int(value), PrimitiveType::Long) => Datum::long(*value), | ||
| (PrimitiveLiteral::Float(value), PrimitiveType::Double) => Datum::double(value.0), | ||
| _ => self.clone(), | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm lesser known with the PyIceberg codebase, but we could also push this down into the projected schema when reading the manifest. This way we always get the current spec.
This avoids having to promote all the values afterwards.
There was a problem hiding this comment.
Thanks for the suggestion @Fokko . This is actually how the Java implementation works (i.e. bind against the current schema).
- The blocker for doing it at manifest parse time is the
ObjectCache: it caches the parsed Manifest keyed by location and survives in-place metadata upgrades. - If we decoded entries under the current/projected schema at parse, we'd bake one scan's schema into that shared, long-lived entry. A later scan could then read stale or wrong-typed partition/bounds from the cache and hit the same false-pruning bug. Keeping the parse a pure function of the manifest's own embedded schema is what keeps the cache sound.
- Comparison-time promotion keeps the cache pure and is consistent with the existing read path: the manifest-list
ManifestEvaluatorcurrently decodesFieldSummarybounds on demand with the predicate's type.
f we want to centralize it, I can project per-scan after the cache (promote each entry's partition tuple + bounds to the current schema in the scan's entry-read loop, so the evaluators drop their promotion). It's cache-safe but is a bit heavy because it rebuilds each entry's partition/bounds in the planning loop, so I prefer the surgical version that the PR currently does. Happy to switch if you think the single-place projection is worth it.
There was a problem hiding this comment.
Got it. I would suggest getting this fix in for now, since it fixes an important issue. Maybe we can create a separate PR to assess the impact of pushing the promotion down to the reader. I think that would simplify it a lot and make it less error prone at the same time.
This branch has not been deployed
2 participants
Which issue does this PR close?
What changes are included in this PR?
A column promoted int->long (or float->double) keeps its originally-written values in manifests produced before the promotion. Partition pruning and stats based pruning can silently drop them, and cause data loss.
This PR fixes both problems. I can split the partition pruning and stats-based pruning into separate PRs if needed (currently two commits in this PR)
Are these changes tested?
Added tests