emkornfield commented on code in PR #1569:
URL: https://github.com/apache/iceberg-rust/pull/1569#discussion_r2337470454
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -1196,14 +1199,92 @@ impl Datum {
(PrimitiveLiteral::Int(val), _, PrimitiveType::Int) =>
Ok(Datum::int(*val)),
(PrimitiveLiteral::Int(val), _, PrimitiveType::Date) =>
Ok(Datum::date(*val)),
(PrimitiveLiteral::Int(val), _, PrimitiveType::Long) =>
Ok(Datum::long(*val)),
+ (PrimitiveLiteral::Int(val), PrimitiveType::Date,
PrimitiveType::Timestamp) => {
+ Ok(Datum::timestamp_micros(*val as i64 *
MICROS_PER_DAY))
+ }
+ (
+ PrimitiveLiteral::Int(val),
+ PrimitiveType::Date,
+ PrimitiveType::Timestamptz,
+ ) => Ok(Datum::timestamptz_micros(*val as i64 *
MICROS_PER_DAY)),
Review Comment:
same comment on safe multiply.
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -3954,4 +4035,301 @@ mod tests {
assert_eq!(double_sorted, double_expected);
}
+
+ #[test]
+ fn test_datum_timestamp_nanos_convert_to_timestamp_micros() {
+ let datum = Datum::timestamp_nanos(12345000);
+
+ let result = datum.to(&Primitive(PrimitiveType::Timestamp)).unwrap();
+
+ let expected = Datum::timestamp_micros(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamp_nanos_convert_to_timestamptz_micros() {
+ let datum = Datum::timestamp_nanos(12345000);
+
+ let result = datum.to(&Primitive(PrimitiveType::Timestamptz)).unwrap();
+
+ let expected = Datum::timestamptz_micros(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamptz_nanos_convert_to_timestamp_micros() {
+ let datum = Datum::timestamptz_nanos(12345000);
+
+ let result = datum.to(&Primitive(PrimitiveType::Timestamp)).unwrap();
+
+ let expected = Datum::timestamp_micros(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamptz_nanos_convert_to_timestamptz_micros() {
Review Comment:
still ramping up the code base, and not sure if we do it elsewhere but would
be possible to parameterize this form of test to avoid the boiler plate?
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -1196,14 +1199,92 @@ impl Datum {
(PrimitiveLiteral::Int(val), _, PrimitiveType::Int) =>
Ok(Datum::int(*val)),
(PrimitiveLiteral::Int(val), _, PrimitiveType::Date) =>
Ok(Datum::date(*val)),
(PrimitiveLiteral::Int(val), _, PrimitiveType::Long) =>
Ok(Datum::long(*val)),
+ (PrimitiveLiteral::Int(val), PrimitiveType::Date,
PrimitiveType::Timestamp) => {
+ Ok(Datum::timestamp_micros(*val as i64 *
MICROS_PER_DAY))
+ }
+ (
+ PrimitiveLiteral::Int(val),
+ PrimitiveType::Date,
+ PrimitiveType::Timestamptz,
+ ) => Ok(Datum::timestamptz_micros(*val as i64 *
MICROS_PER_DAY)),
+ (
+ PrimitiveLiteral::Int(val),
+ PrimitiveType::Date,
+ PrimitiveType::TimestampNs,
+ ) => Ok(Datum::timestamp_nanos(
+ *val as i64 * MICROS_PER_DAY * NANOS_PER_MICRO,
+ )),
+ (
+ PrimitiveLiteral::Int(val),
+ PrimitiveType::Date,
+ PrimitiveType::TimestamptzNs,
+ ) => Ok(Datum::timestamptz_nanos(
+ *val as i64 * MICROS_PER_DAY * NANOS_PER_MICRO,
Review Comment:
safe multiply ... Also, I assume the compiler would optimize this out but
consider just defining NANOS_PER_DAY and multiple that?
##########
crates/integrations/datafusion/src/physical_plan/expr_to_predicate.rs:
##########
@@ -214,18 +218,46 @@ fn scalar_value_to_datum(value: &ScalarValue) ->
Option<Datum> {
ScalarValue::LargeUtf8(Some(v)) => Some(Datum::string(v.clone())),
ScalarValue::Date32(Some(v)) => Some(Datum::date(*v)),
ScalarValue::Date64(Some(v)) => Some(Datum::date((*v / MILLIS_PER_DAY)
as i32)),
+ ScalarValue::TimestampSecond(Some(v), tz) => {
+ interpret_timestamptz_micros(v * MICROS_PER_SECOND, tz.as_deref())
Review Comment:
safe multiply?
##########
crates/integrations/datafusion/src/physical_plan/expr_to_predicate.rs:
##########
@@ -429,4 +497,76 @@ mod tests {
let predicate = convert_to_iceberg_predicate(sql);
assert_eq!(predicate, None);
}
+
+ #[test]
+ fn test_predicate_conversion_with_timestamp() {
+ // 2023-01-01 12:00:00 UTC
+ let timestamp_scalar = ScalarValue::TimestampSecond(Some(1672574400),
None);
+ let dt =
DateTime::parse_from_rfc3339("2023-01-01T12:00:00+00:00").unwrap();
Review Comment:
same comment about maybe considering parameterization. of these tests.
##########
crates/iceberg/src/spec/values.rs:
##########
@@ -3954,4 +4035,301 @@ mod tests {
assert_eq!(double_sorted, double_expected);
}
+
+ #[test]
+ fn test_datum_timestamp_nanos_convert_to_timestamp_micros() {
+ let datum = Datum::timestamp_nanos(12345000);
+
+ let result = datum.to(&Primitive(PrimitiveType::Timestamp)).unwrap();
+
+ let expected = Datum::timestamp_micros(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamp_nanos_convert_to_timestamptz_micros() {
+ let datum = Datum::timestamp_nanos(12345000);
+
+ let result = datum.to(&Primitive(PrimitiveType::Timestamptz)).unwrap();
+
+ let expected = Datum::timestamptz_micros(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamptz_nanos_convert_to_timestamp_micros() {
+ let datum = Datum::timestamptz_nanos(12345000);
+
+ let result = datum.to(&Primitive(PrimitiveType::Timestamp)).unwrap();
+
+ let expected = Datum::timestamp_micros(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamptz_nanos_convert_to_timestamptz_micros() {
+ let datum = Datum::timestamptz_nanos(12345000);
+
+ let result = datum.to(&Primitive(PrimitiveType::Timestamptz)).unwrap();
+
+ let expected = Datum::timestamptz_micros(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamp_micros_convert_to_timestamp_nanos() {
+ let datum = Datum::timestamp_micros(12345);
+
+ let result = datum.to(&Primitive(PrimitiveType::TimestampNs)).unwrap();
+
+ let expected = Datum::timestamp_nanos(12345000);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamp_micros_convert_to_timestamptz_nanos() {
+ let datum = Datum::timestamp_micros(12345);
+
+ let result =
datum.to(&Primitive(PrimitiveType::TimestamptzNs)).unwrap();
+
+ let expected = Datum::timestamptz_nanos(12345000);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamptz_micros_convert_to_timestamp_nanos() {
+ let datum = Datum::timestamptz_micros(12345);
+
+ let result = datum.to(&Primitive(PrimitiveType::TimestampNs)).unwrap();
+
+ let expected = Datum::timestamp_nanos(12345000);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamptz_micros_convert_to_timestamptz_nanos() {
+ let datum = Datum::timestamptz_micros(12345);
+
+ let result =
datum.to(&Primitive(PrimitiveType::TimestamptzNs)).unwrap();
+
+ let expected = Datum::timestamptz_nanos(12345000);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamp_nanos_convert_to_timestamp_nanos() {
+ let datum = Datum::timestamp_nanos(12345);
+
+ let result = datum.to(&Primitive(PrimitiveType::TimestampNs)).unwrap();
+
+ let expected = Datum::timestamp_nanos(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamp_nanos_convert_to_timestamptz_nanos() {
+ let datum = Datum::timestamp_nanos(12345);
+
+ let result =
datum.to(&Primitive(PrimitiveType::TimestamptzNs)).unwrap();
+
+ let expected = Datum::timestamptz_nanos(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamptz_nanos_convert_to_timestamp_nanos() {
+ let datum = Datum::timestamptz_nanos(12345);
+
+ let result = datum.to(&Primitive(PrimitiveType::TimestampNs)).unwrap();
+
+ let expected = Datum::timestamp_nanos(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamptz_nanos_convert_to_timestamptz_nanos() {
+ let datum = Datum::timestamptz_nanos(12345);
+
+ let result =
datum.to(&Primitive(PrimitiveType::TimestamptzNs)).unwrap();
+
+ let expected = Datum::timestamptz_nanos(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_long_convert_to_timestamp_nanos() {
+ let datum = Datum::long(12345);
+
+ let result = datum.to(&Primitive(PrimitiveType::TimestampNs)).unwrap();
+
+ let expected = Datum::timestamp_nanos(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_long_convert_to_timestamptz_nanos() {
+ let datum = Datum::long(12345);
+
+ let result =
datum.to(&Primitive(PrimitiveType::TimestamptzNs)).unwrap();
+
+ let expected = Datum::timestamptz_nanos(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamp_nanos_to_micros() {
+ let datum = Datum::timestamp_nanos(12345678);
+
+ let result = datum.to(&Primitive(PrimitiveType::Timestamp)).unwrap();
+
+ let expected = Datum::timestamp_micros(12345);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_timestamp_micros_to_nanos() {
+ let datum = Datum::timestamp_micros(12345);
+
+ let result = datum.to(&Primitive(PrimitiveType::TimestampNs)).unwrap();
+
+ let expected = Datum::timestamp_nanos(12345000);
+
+ assert_eq!(result, expected);
+ }
+
+ #[test]
+ fn test_datum_date_convert_to_timestamp() {
+ let datum = Datum::date(1); // 1970-01-02
+
+ let result = datum.to(&Primitive(PrimitiveType::Timestamp)).unwrap();
+
+ let expected = Datum::timestamp_from_datetime(
Review Comment:
It would probably be useful to test with wider ranges (i.e. year 9999-12-31
and year 0001-01-01) as well.
--
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]
