mbutrovich commented on code in PR #3574:
URL: https://github.com/apache/iceberg-rust/pull/3574#discussion_r4428430700
##########
crates/iceberg/src/spec/values/datum.rs:
##########
@@ -1312,3 +1306,104 @@ fn human_float<T: Float + LowerExp + FromStr>(value: T)
-> String {
format!("{sign}{}.{fraction}E{exponent}", &digits[..1])
}
}
+
+const MICROS_PER_DAY: i64 = 24 * 60 * 60 * 1_000_000;
+const NANOS_PER_MICRO: i64 = 1000;
+const NANOS_PER_DAY: i64 = NANOS_PER_MICRO * MICROS_PER_DAY;
+
+/// Converts an int literal between two [PrimitiveType]s.
+fn convert_int(
+ val: i32,
+ source_type: &PrimitiveType,
+ target_type: &PrimitiveType,
+) -> Result<Datum> {
+ let datum = match (source_type, target_type) {
+ (_, PrimitiveType::Int) => Datum::int(val),
+ (_, PrimitiveType::Date) => Datum::date(val),
+ (_, PrimitiveType::Long) => Datum::long(val),
+ (PrimitiveType::Date, PrimitiveType::Timestamp) =>
Datum::timestamp_micros(
+ (val as i64)
+ .checked_mul(MICROS_PER_DAY)
+ .ok_or_else(overflow)?,
+ ),
+ (PrimitiveType::Date, PrimitiveType::Timestamptz) =>
Datum::timestamptz_micros(
+ (val as i64)
+ .checked_mul(MICROS_PER_DAY)
+ .ok_or_else(overflow)?,
+ ),
+ (PrimitiveType::Date, PrimitiveType::TimestampNs) =>
Datum::timestamp_nanos(
+ (val as i64)
+ .checked_mul(NANOS_PER_DAY)
+ .ok_or_else(overflow)?,
+ ),
+ (PrimitiveType::Date, PrimitiveType::TimestamptzNs) =>
Datum::timestamptz_nanos(
+ (val as i64)
+ .checked_mul(NANOS_PER_DAY)
+ .ok_or_else(overflow)?,
+ ),
+ _ => {
+ return Err(invalid_data!(
+ "Can't convert datum from {source_type} type to {target_type}
type."
+ ));
+ }
+ };
+
+ Ok(datum)
+}
+
+/// Converts a long literal between two [PrimitiveType]s.
+fn convert_long(
+ val: i64,
+ source_type: &PrimitiveType,
+ target_type: &PrimitiveType,
+) -> Result<Datum> {
+ let datum = match (source_type, target_type) {
+ (_, PrimitiveType::Int) => Datum::i64_to_i32(val),
+ (_, PrimitiveType::Long) => Datum::long(val),
+ (
+ PrimitiveType::Long | PrimitiveType::Timestamp |
PrimitiveType::Timestamptz,
+ PrimitiveType::Timestamp,
+ ) => Datum::timestamp_micros(val),
+ (
+ PrimitiveType::Long | PrimitiveType::Timestamp |
PrimitiveType::Timestamptz,
+ PrimitiveType::Timestamptz,
+ ) => Datum::timestamptz_micros(val),
+ (
+ PrimitiveType::Long | PrimitiveType::TimestampNs |
PrimitiveType::TimestamptzNs,
+ PrimitiveType::TimestampNs,
+ ) => Datum::timestamp_nanos(val),
+ (
+ PrimitiveType::Long | PrimitiveType::TimestampNs |
PrimitiveType::TimestamptzNs,
+ PrimitiveType::TimestamptzNs,
+ ) => Datum::timestamptz_nanos(val),
+ (PrimitiveType::TimestampNs | PrimitiveType::TimestamptzNs,
PrimitiveType::Timestamp) => {
+ Datum::timestamp_micros(val.div_euclid(NANOS_PER_MICRO))
+ }
+ (PrimitiveType::TimestampNs | PrimitiveType::TimestamptzNs,
PrimitiveType::Timestamptz) => {
+ Datum::timestamptz_micros(val.div_euclid(NANOS_PER_MICRO))
+ }
+ (PrimitiveType::Timestamp | PrimitiveType::Timestamptz,
PrimitiveType::TimestampNs) => {
+
Datum::timestamp_nanos(val.checked_mul(NANOS_PER_MICRO).ok_or_else(overflow)?)
+ }
+ (PrimitiveType::Timestamp | PrimitiveType::Timestamptz,
PrimitiveType::TimestamptzNs) => {
+
Datum::timestamptz_nanos(val.checked_mul(NANOS_PER_MICRO).ok_or_else(overflow)?)
+ }
+ (PrimitiveType::Timestamp | PrimitiveType::Timestamptz,
PrimitiveType::Date) => {
+ Datum::date(val.div_euclid(MICROS_PER_DAY) as i32)
+ }
+ (PrimitiveType::TimestampNs | PrimitiveType::TimestamptzNs,
PrimitiveType::Date) => {
+ Datum::date(val.div_euclid(NANOS_PER_DAY) as i32)
+ }
Review Comment:
> Seems much better to fail than to do a lossy conversion, and whether to
deviate from the Java impl is your call :)
Not my call, just my reading :) Happy to get more feedback @blackmwk @CTTY
@laskoviymishka. In particular, I'm looking at the [expressions
spec](https://github.com/apache/iceberg/blob/dfd7ef3b711cd1977b93f4e6f86cb4148712acd3/format/expressions-spec.md?plain=1#L98)
("implementations must fail rather than insert unsafe casts"). Java's flooring
dates from 2018, and the expressions spec was added this year, so as I read it
Java hasn't caught up yet.
> Does going from `timestamptz` to `timestamp` count as narrowing to you?
Not narrowing, but as I read the spec it's the same problem as `date` to
`timestamptz`. A [`timestamp` is a wall-clock value and a `timestamptz` is an
instant](https://github.com/apache/iceberg/blob/dfd7ef3b711cd1977b93f4e6f86cb4148712acd3/format/spec.md?plain=1#L300-L301),
so converting between them needs a zone, and these arms assume UTC. That is
why the [promotion
table](https://github.com/apache/iceberg/blob/dfd7ef3b711cd1977b93f4e6f86cb4148712acd3/format/spec.md?plain=1#L402)
forbids `date` to `timestamptz`. Java doesn't help here.
[`TimestampType.typeId()`](https://github.com/apache/iceberg/blob/dfd7ef3b711cd1977b93f4e6f86cb4148712acd3/api/src/main/java/org/apache/iceberg/types/Types.java#L246-L271)
returns `TIMESTAMP` for both zones, so
[`TimestampLiteral.to`](https://github.com/apache/iceberg/blob/dfd7ef3b711cd1977b93f4e6f86cb4148712acd3/api/src/main/java/org/apache/iceberg/expressions/Literals.java#L445-L448)
can't tell them apart.
Could the new arms in
[`convert_long`](https://github.com/apache/iceberg-rust/blob/1ff988401a654aa709e59e63d70f40c8267e4dd8/crates/iceberg/src/spec/values/datum.rs#L1357-L1369)
keep the zone? That means `timestamp_ns` to `timestamp_ns`, `timestamptz_ns`
to `timestamptz_ns`, `timestamp` to `timestamp_ns`, and `timestamptz` to
`timestamptz_ns`. On `main` the `timestamp_ns` and `timestamptz_ns` cases
return an error, so this doesn't take anything away.
You were right that the microsecond case is pre-existing. On `main`, any
`long`-backed literal converts to `timestamp` or `timestamptz`
([datum.rs#L1147-L1152](https://github.com/apache/iceberg-rust/blob/4dd8c946432f5e56a3ccc88e5b438a356ba28287/crates/iceberg/src/spec/values/datum.rs#L1147-L1152)).
`datafusion-iceberg` depends on it, because it maps a zone-aware
`TimestampMicrosecond` to `Datum::timestamp_micros`
([predicate.rs#L465-L467](https://github.com/apache/datafusion-iceberg/blob/8979482936eff6509e40c2e4e3bfc1235d965d60/crates/datafusion/src/read/predicate.rs#L465-L467)).
Changing the two microsecond arms would break that mapping, so could they stay
as they are in this PR?
--
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]
