ump45nose commented on code in PR #3522:
URL: https://github.com/apache/iceberg-rust/pull/3522#discussion_r4401228234
##########
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:
Good catch — handled. `Datum::try_from_bytes` now takes the 4-byte path for
`Timestamp` / `TimestampNs` the same way `Long` does and reuses
`date_to_timestamp`. Added
`test_parse_manifest_promotes_date_bounds_under_timestamp_schema` next to
`test_parse_manifest_promotes_writer_types`, plus
`avro_bytes_timestamp_from_date` for the byte-level round-trip.
(AI-assisted reply.)
##########
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:
Agreed — dropped the partition half. Removed the date→timestamp/timestamp_ns
arm, the `promoted_partition` change, and its tests so the PR stays within what
the spec allows for identity partitions. Bound promotion only.
(AI-assisted reply.)
##########
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:
Done. Folded the `<` / `>` / `=` / `IN` pruning cases into one table-driven
`eval_promotes_date_bounds` with an inside (kept) and outside (pruned) literal
for both `timestamp` and `timestamp_ns`. Overflow test stays separate and now
asserts `ErrorKind::DataInvalid`. Merged the datum promote tests into
`test_datum_promote_date` and added the `timestamp_ns` edges (day 106751 fits /
106752 doesn't).
(AI-assisted reply.)
##########
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:
Done — `promote_to` now matches on `(self.data_type(), target)`, so a bare
`Int` no longer promotes as a date.
(AI-assisted reply.)
--
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]
