mbutrovich commented on code in PR #3574:
URL: https://github.com/apache/iceberg-rust/pull/3574#discussion_r4419570383
##########
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),
Review Comment:
Could this arm go? `(_, PrimitiveType::Long)` is new for non-`long` sources,
so `Datum::timestamp_micros(5).to(Long)` and
`Datum::timestamp_nanos(5).to(Long)` both return `Ok(Datum::long(5))` on the
head commit. On `main` they return an error. The result depends on the unit of
the source type, and `long` to `long` is already handled by the `self_type ==
target_type` arm at line 1135.
##########
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:
Should these narrowing arms (`timestamp_ns` to `timestamp`, and any
timestamp to `date`) return an error instead? `Datum::to` is only called from
predicate binding
([predicate.rs#L419](https://github.com/apache/iceberg-rust/blob/a321be51a9714fccc2efacb704482f4262d6e368/crates/iceberg/src/expr/predicate.rs#L419)
and
[#L462](https://github.com/apache/iceberg-rust/blob/a321be51a9714fccc2efacb704482f4262d6e368/crates/iceberg/src/expr/predicate.rs#L462)),
and flooring the literal changes the predicate. On the head commit, binding
`Reference::new("d").less_than(Datum::timestamp_micros(1_672_920_000_000_000))`
(2023-01-05 12:00) to a `date` column gives `d < 2023-01-05` (`Int(19362)`). A
row with `d = 2023-01-05` matches the original predicate, because midnight is
before noon, but not the bound one. The same holds for `ts < 1500ns` bound to a
`timestamp` column, which becomes `ts < 1us` and drops `ts = 1us`. The
`datafusion-iceberg` description for apache/datafusion-iceberg#91 describes
the same result for this query.
The [expressions
spec](https://github.com/apache/iceberg/blob/c71e7dfca17633ae2bce15c486278e8e07629cea/format/expressions-spec.md?plain=1#L127)
covers this case:
> If the types cannot be aligned according to type promotion rules (for
instance, `"goats" > -Infinity`), the predicate cannot be evaluated and
implementations must fail.
and
[L98](https://github.com/apache/iceberg/blob/c71e7dfca17633ae2bce15c486278e8e07629cea/format/expressions-spec.md?plain=1#L98)
says "implementations must fail rather than insert unsafe casts". Timestamp to
date isn't a promotion. The aligned comparison would promote the date column to
a timestamp, which binding can't express because it converts the literal to the
column type. Java floors here
([Literals.java#L445-L455](https://github.com/apache/iceberg/blob/c71e7dfca17633ae2bce15c486278e8e07629cea/api/src/main/java/org/apache/iceberg/expressions/Literals.java#L445-L455),
[#L471-L480](https://github.com/apache/iceberg/blob/c71e7dfca17633ae2bce15c486278e8e07629cea/api/src/main/java/org/apache/iceberg/expressions/Literals.java#L471-L480)),
which as I read it contradicts the spec.
Returning the error here also fixes the `main` bug, so the user-visible
change becomes an error instead of a wrong answer. Once
apache/datafusion-iceberg#91 lands, the query in the description no longer
pushes the cast down, so it doesn't reach this code. Is there another caller
that needs the timestamp to date conversion?
##########
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),
Review Comment:
Should a `long` literal bound to a `timestamp_ns` column return an error
instead? With these arms, `Datum::long(1).to(TimestampNs)` returns 1 ns. Java
reads the long as micros and returns 1000 ns
([Literals.java#L309-L311](https://github.com/apache/iceberg/blob/c71e7dfca17633ae2bce15c486278e8e07629cea/api/src/main/java/org/apache/iceberg/expressions/Literals.java#L309-L311)).
`long` isn't promotable to any timestamp type, so as I read the spec the error
is the answer (see the comment on lines 1379-1396). If the arm stays, I'd
follow Java's unit.
The comment at line 1148 ("Let's wait with nano's until this clears up",
pointing at apache/iceberg#11775) is stale either way. That PR was closed, and
this PR adds the nano conversions. Could we remove it?
##########
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)?,
+ ),
Review Comment:
Should date to `timestamptz` and `timestamptz_ns` stay unsupported? The
[type promotion
table](https://github.com/apache/iceberg/blob/c71e7dfca17633ae2bce15c486278e8e07629cea/format/spec.md?plain=1#L396-L404)
allows `date` to `timestamp` and `timestamp_ns`, and says "Promotion to
`timestamptz` or `timestamptz_ns` is **not** allowed". These arms read the date
as midnight UTC, but a SQL engine reads `DATE '2023-01-05'` compared with a
`timestamptz` as midnight in the session time zone. I'd keep the two
`timestamp` arms and drop the two `timestamptz` arms.
--
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]
