anoopj commented on code in PR #3483:
URL: https://github.com/apache/iceberg-rust/pull/3483#discussion_r4307476952
##########
crates/iceberg/src/spec/values/datum.rs:
##########
@@ -1229,6 +1229,19 @@ impl Datum {
&self.r#type
}
+ /// 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(),
+ }
+ }
Review 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 `ManifestEvaluator` currently decodes
`FieldSummary` bounds 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.
--
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]
