ewoodbury commented on code in PR #3522:
URL: https://github.com/apache/iceberg-rust/pull/3522#discussion_r4398127708
##########
crates/iceberg/src/spec/values/primitive.rs:
##########
@@ -159,3 +176,23 @@ impl PrimitiveLiteral {
}
}
}
+
+const MICROSECONDS_PER_DAY: i64 = 86_400_000_000;
+const NANOSECONDS_PER_DAY: i64 = 86_400_000_000_000;
+
+/// Converts `days` from the epoch to a timestamp literal in units of
`units_per_day`.
+fn date_to_timestamp(
+ days: i32,
+ units_per_day: i64,
+ target: &PrimitiveType,
+) -> Result<PrimitiveLiteral> {
+ i64::from(days)
+ .checked_mul(units_per_day)
+ .map(PrimitiveLiteral::Long)
+ .ok_or_else(|| {
+ Error::new(
Review Comment:
nit: `invalid_data!` would match the rest of the crate. The error also
doesn't say which column failed. Adding the field name in `promote_bound` would
make a failed scan easier to track down.
##########
crates/iceberg/src/scan/context.rs:
##########
@@ -91,6 +92,9 @@ fn partition_types_differ(current: &StructType, written:
&StructType) -> bool {
) | (
Some(PrimitiveType::Float),
Some(PrimitiveType::Double | PrimitiveType::Unknown)
+ ) | (
+ Some(PrimitiveType::Date),
+ Some(PrimitiveType::Timestamp | PrimitiveType::TimestampNs)
Review Comment:
The spec doesn't allow promoting a partition source when the transform's
value would change, and it lists `date` to `timestamp`/`timestamp_ns` as such a
case. `day`, `month` and `year` keep the same value and type, so the only
partition field this arm can match is `identity`. That is the case the spec
rules out, so the arm only fires for tables that broke the rule.
Is the intent to tolerate those tables? If so, I'd say so in the doc comment
and add a scan-level test like #3483's
`test_scan_widens_old_partition_so_pruning_and_deletes_match`. Today the full
lib suite still passes with this arm removed, so the delete-matching claim
isn't covered. Otherwise, I'd lean toward dropping the partition half (this
arm, the `promoted_partition` change and its tests) to keep the PR to what the
spec allows. What do you think?
##########
crates/iceberg/src/spec/values/primitive.rs:
##########
@@ -159,3 +176,23 @@ impl PrimitiveLiteral {
}
}
}
+
+const MICROSECONDS_PER_DAY: i64 = 86_400_000_000;
+const NANOSECONDS_PER_DAY: i64 = 86_400_000_000_000;
+
+/// Converts `days` from the epoch to a timestamp literal in units of
`units_per_day`.
+fn date_to_timestamp(
Review Comment:
This covers manifests written before the promotion, where the bound is
decoded as `date`. The spec's bounds table also covers manifests written after
it: under a `timestamp` or `timestamp_ns` schema, a 4-byte bound is a `date`.
Promotion doesn't rewrite bounds, so a manifest that carries an older file
forward (a merged manifest, for example) holds exactly this.
`Datum::try_from_bytes` handles the 4-byte `long` and `double` rows but not
these, so the whole manifest fails to load. With `ts: timestamp` and a
`Datum::date(19_723)` bound, `Manifest::parse_avro` returns `DataInvalid =>
failed to convert byte slice to array`. `timestamp_ns` fails the same way.
I'd handle it here, since #3503 covers decoding bounds by their written
type. The `Timestamp` and `TimestampNs` arms could take the 4-byte case the way
`Long` does and reuse `date_to_timestamp`. A round-trip test next to
`test_parse_manifest_promotes_writer_types` would pin it.
##########
crates/iceberg/src/spec/values/datum.rs:
##########
@@ -1230,14 +1230,20 @@ impl Datum {
}
/// 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. Returns `None` when `target` does not widen this value (decimals
- /// already compare across precisions), so callers can avoid a copy.
- pub(crate) fn promote_to(&self, target: &PrimitiveType) -> Option<Datum> {
- self.literal
- .promote_to(target)
- .map(|literal| Datum::new(target.clone(), literal))
+ /// promotions `int` -> `long`, `float` -> `double`, and `date` ->
`timestamp` /
+ /// `timestamp_ns`, so a value written under the narrower type compares
against
+ /// a predicate bound to the wider one. Returns `Ok(None)` when `target`
does not
+ /// widen this value (decimals already compare across precisions), so
callers
+ /// can avoid a copy.
+ ///
+ /// # Errors
+ /// Returns `DataInvalid` if a date is outside the range of the target
timestamp
+ /// type.
+ pub(crate) fn promote_to(&self, target: &PrimitiveType) ->
Result<Option<Datum>> {
Review Comment:
Not blocking: `Datum` knows its own type, so I'd match on
`(self.data_type(), target)` here. Right now any `Int` literal is treated as a
date: `Datum::int(5).promote_to(&PrimitiveType::Timestamp)` returns a timestamp
5 days after the epoch. No caller does that today, but matching on the type
keeps a future caller from hitting it.
##########
crates/iceberg/src/expr/visitors/inclusive_metrics_evaluator.rs:
##########
@@ -627,6 +631,92 @@ mod test {
assert!(InclusiveMetricsEvaluator::eval(&predicate, &data_file,
true).unwrap());
}
+ // 19_723 days after the epoch is 2024-01-01.
+ const MICROS_PER_DAY: i64 = 86_400_000_000;
+
+ #[test]
+ fn eval_promotes_date_bounds_for_inequality() {
Review Comment:
The `<`/`>` and `IN` tests only check that a file is kept. If I make those
two paths keep every file with a date bound, the whole suite still passes. Only
the `=` test checks that a file gets pruned.
I'd fold the three pruning tests into one table-driven test. Each operator
would get a literal inside the bounds (kept) and one outside (pruned), for both
`timestamp` and `timestamp_ns`. That covers the pruned side and should come out
shorter than the current tests. The overflow test can stay separate, but I'd
assert `ErrorKind::DataInvalid` in place of `is_err()`. The two
`test_datum_promote_*` tests in `spec/values/tests.rs` could merge into one
table the same way. If you do, two more rows would pin the exact `timestamp_ns`
edge: day 106751 fits and 106752 doesn't.
--
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]
