fix: handle more types of timestamp conversion by colinmarc · Pull Request #3574 · apache/iceberg-rust

colinmarc · 2026-10-29T09:28:10Z

This adds support for converting date values, which helps with predicate expressions such as date > DATE_TRUNC('day', ts).

For example, without this, the following datafusion query fails at runtime:

SELECT id FROM p WHERE d < TIMESTAMP '2023-01-05 12:00:00'
-- IcebergError(DataInvalid): Can't convert datum from timestamp_ns type to date type

Which issue does this PR close?

This was originally part of #1569.

Are these changes tested?

Yes! I also restructured the datum convert tests, as requested by feedback in #1569.

AI Disclosure

I wrote this code myself a while ago, but used a model to rebase it.

NB

This is the only change requiring us to stay on a fork, so I'd really like to get it in.

colinmarc · 2026-10-29T09:36:22Z

Sorry, I initially pushed the non-rebased version. Fixed now.

mbutrovich

Thanks @colinmarc for splitting this out of #1569 and for restructuring the tests. This also fixes a bug on main, where binding a timestamp_ns literal to a timestamp column keeps the nanosecond value as microseconds (datum.rs#L1147-L1152).

mbutrovich · 2026-10-30T01:21:18Z

+        (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)?,
+        ),


Should date to timestamptz and timestamptz_ns stay unsupported? The type promotion table 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.

It makes sense not to silently introduce UTC. I think I was working off the java code originally, but I'm not sure.

What about timestamp to timestamptz? That seems like the same situation, but it's preexisting.

Edit: hm, maybe it's not preexisting, actually. I got mixed up. I'll remove them for now.

mbutrovich · 2026-10-30T01:21:27Z

+) -> Result<Datum> {
+    let datum = match (source_type, target_type) {
+        (_, PrimitiveType::Int) => Datum::i64_to_i32(val),
+        (_, PrimitiveType::Long) => Datum::long(val),


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.

Removed it.

mbutrovich · 2026-10-30T01:21:41Z

+        (
+            PrimitiveType::Long | PrimitiveType::TimestampNs | PrimitiveType::TimestamptzNs,
+            PrimitiveType::TimestampNs,
+        ) => Datum::timestamp_nanos(val),
+        (
+            PrimitiveType::Long | PrimitiveType::TimestampNs | PrimitiveType::TimestamptzNs,
+            PrimitiveType::TimestamptzNs,
+        ) => Datum::timestamptz_nanos(val),


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). 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?

I removed it.

mbutrovich · 2026-10-30T01:22:06Z

+        (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)
+        }


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 and #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 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 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, #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?

Seems much better to fail than to do a lossy conversion, and whether to deviate from the Java impl is your call :)

I agree that this is not as critical if apache/datafusion-iceberg#91 lands, though.

Does going from timestamptz to timestamp count as narrowing to you?

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 ("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, so converting between them needs a zone, and these arms assume UTC. That is why the promotion table forbids date to timestamptz. Java doesn't help here. TimestampType.typeId() returns TIMESTAMP for both zones, so TimestampLiteral.to can't tell them apart.

Could the new arms in convert_long 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). datafusion-iceberg depends on it, because it maps a zone-aware TimestampMicrosecond to Datum::timestamp_micros (predicate.rs#L465-L467). Changing the two microsecond arms would break that mapping, so could they stay as they are in this PR?

+1 to failing on the narrowing arms. The spec is explicit ("must fail rather than insert unsafe casts"), and an error is better than the wrong answer we return on main today.

Since this means Rust deliberately diverges from Java's flooring in Literals.java, could we raise it upstream so the clients don't drift apart? Either a short dev-list thread or an issue/PR in apache/iceberg.

If Java is wrong here, it should be fixed there too. If not, we should understand why.

This adds support for converting date values, which helps with predicate expressions such as `date > DATE_TRUNC('day', ts)`.

mbutrovich

Thanks @colinmarc for the quick turnaround.

mbutrovich · 2026-10-30T21:01:10Z

-                    (PrimitiveLiteral::Long(val), _, PrimitiveType::Timestamp) => {
-                        Ok(Datum::timestamp_micros(*val))
-                    }
-                    (PrimitiveLiteral::Long(val), _, PrimitiveType::Timestamptz) => {
-                        Ok(Datum::timestamptz_micros(*val))
-                    }


Could the PR description mention this removal? Datum::long(12345).to(&Primitive(PrimitiveType::Timestamp)) returns timestamp_micros(12345) on main, and test_datum_long_convert_to_timestamp checked that. On the head commit it returns "Can't convert datum from long type to timestamp type." The same applies to timestamptz. That matches my reading of the spec, but a caller binding a long literal to a timestamp column now gets an error, and the description only mentions what the PR adds.

mbutrovich · 2026-10-30T21:01:29Z

-    let datum = Datum::string("12345");
-
-    let result = datum.to(&Primitive(PrimitiveType::Int)).unwrap();
-
-    let expected = Datum::int(12345);
-
-    assert_eq!(result, expected);
-}
-
-#[test]
-fn test_datum_string_convert_to_long() {
-    let datum = Datum::string("12345");
-
-    let result = datum.to(&Primitive(PrimitiveType::Long)).unwrap();
-
-    let expected = Datum::long(12345);
-
-    assert_eq!(result, expected);
-}
-
-#[test]
-fn test_datum_string_convert_to_timestamp() {
-    let datum = Datum::string("1925-05-20T19:25:00.000");
-


Could these tests check the values at and just past each boundary? i32::MAX / 2 and i64::MAX / 10 are far past it, so an off-by-one in the range check would still pass. On the head commit, Datum::date(106751) and Datum::date(-106751) convert to timestamp_ns, and Datum::date(106752) and Datum::date(-106752) return the overflow error. For micros to nanos, i64::MAX / 1000 converts and i64::MAX / 1000 + 1 overflows, and the negative side has no test yet. The promotion table requires the failure for values outside the promoted type's range.

github-actions Bot added the datafusion label Oct 29, 2026

colinmarc changed the title ~~fix(iceberg-datafusion): handle more types of timestamp conversion~~ fix: handle more types of timestamp conversion Oct 29, 2026

colinmarc force-pushed the colin/datum-conversions-2 branch from deeaf06 to a321be5 Compare October 29, 2026 09:35

colinmarc mentioned this pull request Oct 29, 2026

fix: refuse to look inside casts in expr_to_predicate apache/datafusion-iceberg#91

Open

mbutrovich self-requested a review October 30, 2026 01:03

mbutrovich reviewed Oct 30, 2026

View reviewed changes

fix(iceberg-datafusion): handle more types of timestamp conversion

1ff9884

This adds support for converting date values, which helps with predicate expressions such as `date > DATE_TRUNC('day', ts)`.

colinmarc force-pushed the colin/datum-conversions-2 branch from a321be5 to 1ff9884 Compare October 30, 2026 12:13

mbutrovich reviewed Oct 30, 2026

View reviewed changes

Conversation

colinmarc commented Oct 29, 2026

Which issue does this PR close?

Are these changes tested?

AI Disclosure

NB

Uh oh!

colinmarc commented Oct 29, 2026

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colinmarc Oct 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

colinmarc Oct 30, 2026 •

edited

Loading