Baunsgaard commented on code in PR #3237:
URL: https://github.com/apache/iceberg-rust/pull/3237#discussion_r4412923741
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -155,6 +179,7 @@ impl Month {
impl TransformFunction for Month {
fn transform(&self, input: ArrayRef) -> Result<ArrayRef> {
+ let input = timestamp_array_in_utc(input);
Review Comment:
same as the above
[comment](https://github.com/apache/iceberg-rust/pull/3237/changes#r4412904623)
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -67,6 +90,7 @@ impl Year {
impl TransformFunction for Year {
fn transform(&self, input: ArrayRef) -> Result<ArrayRef> {
+ let input = timestamp_array_in_utc(input);
Review Comment:
nit: this shadows the parameter, so later `&input` is the retagged array and
the original is gone.
A distinct name makes that obvious.
```rust
let utc_input = timestamp_array_in_utc(input);
let array = date_part(&utc_input, DatePart::Year)
```
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -41,6 +41,29 @@ const MICROS_PER_SECOND: i64 = 1_000_000;
/// One second in nanos.
const NANOS_PER_SECOND: i64 = 1_000_000_000;
+/// Re-tag timestamp arrays as UTC so calendar fields match Iceberg's
+/// epoch-based computation. `date_part` converts into the array's timezone
+/// tag before reading year/month; Iceberg always uses UTC.
+fn timestamp_array_in_utc(input: ArrayRef) -> ArrayRef {
+ match input.data_type() {
+ DataType::Timestamp(TimeUnit::Microsecond, Some(_)) => {
+ let arr = input
+ .as_any()
+ .downcast_ref::<TimestampMicrosecondArray>()
+ .unwrap();
+ Arc::new(arr.clone().with_timezone("+00:00"))
+ }
+ DataType::Timestamp(TimeUnit::Nanosecond, Some(_)) => {
+ let arr = input
+ .as_any()
+ .downcast_ref::<TimestampNanosecondArray>()
+ .unwrap();
+ Arc::new(arr.clone().with_timezone("+00:00"))
+ }
+ _ => input,
+ }
+}
Review Comment:
Iceberg itself only has micro and nano (`Timestamp` / `Timestamptz` /
`*Ns`). `arrow_to_schema` rejects second and millisecond. `Year::transform` /
`Month::transform` do not: they pass the array straight to `date_part`, which
handles those units and will still shift them by the tag. This bug is for
external callers, so if those arrays get here they should be retagged too.
```rust
fn timestamp_array_in_utc(input: ArrayRef) -> ArrayRef {
match input.data_type() {
DataType::Timestamp(unit, Some(_)) => {
let utc = DataType::Timestamp(*unit,
Some(crate::arrow::UTC_TIME_ZONE.into()));
arrow_cast::cast(&input, &utc).expect("timezone-only timestamp
cast")
}
_ => input,
}
}
```
(`arrow-cast` is already a dependency. A timezone-only cast is metadata,
same as `clone().with_timezone`.)
##########
crates/iceberg/src/transform/temporal.rs:
##########
@@ -2374,6 +2399,21 @@ mod test {
assert_eq!(res.value(4), -1);
}
+ #[test]
+ fn test_year_and_month_ignore_non_utc_timezone_tag() {
+ // -1 micros is 1969-12-31T23:59:59.999999Z. A non-UTC tag must not
+ // shift that into 1970 when computing Iceberg year/month values.
+ let tagged: ArrayRef =
+
Arc::new(TimestampMicrosecondArray::from(vec![-1i64]).with_timezone("Asia/Kathmandu"));
+ let years = super::Year.transform(tagged.clone()).unwrap();
+ let years = years.as_any().downcast_ref::<Int32Array>().unwrap();
+ assert_eq!(years.value(0), -1);
+
+ let months = super::Month.transform(tagged).unwrap();
+ let months = months.as_any().downcast_ref::<Int32Array>().unwrap();
+ assert_eq!(months.value(0), -1);
+ }
Review Comment:
Could this use `"+05:45"` instead of `Asia/Kathmandu`? (to resolve the sting
it requires an extra dependency `chrono-tz` that are allreay used in CI, but
not nessesarily when running locally) Same offset, and the assertion is then
clearly "UTC year/month, not local." It would also be good to cover:
- a `TimestampNanosecondArray` (the nanosecond arm is currently untested);
- a negative offset: `0` tagged `"-05:00"` should stay year/month `0`;
- `+00:00` as the control: `-1` is already year/month `-1` with no retag.
Something like:
```rust
for tz in ["+05:45", "-05:00", "+00:00"] {
let micros: ArrayRef =
Arc::new(TimestampMicrosecondArray::from(vec![-1i64,
0]).with_timezone(tz));
let nanos: ArrayRef =
Arc::new(TimestampNanosecondArray::from(vec![-1i64,
0]).with_timezone(tz));
for input in [micros, nanos] {
let years = super::Year.transform(input.clone()).unwrap();
let years = years.as_any().downcast_ref::<Int32Array>().unwrap();
// Iceberg year is UTC: -1 micros is always -1, epoch is always 0.
// +00:00 already did this; the other tags must match it.
assert_eq!(years.values(), &[-1, 0], "year, tz={tz}");
let months = super::Month.transform(input).unwrap();
let months = months.as_any().downcast_ref::<Int32Array>().unwrap();
assert_eq!(months.values(), &[-1, 0], "month, tz={tz}");
}
}
--
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]
