fix(iceberg-datafusion): handle timestamp predicates from DF by colinmarc · Pull Request #1569 · apache/iceberg-rust

colinmarc · 2025-07-31T10:59:09Z

DataFusion sometimes passes dates as string literals, but can also pass timestamp ScalarValues, which need to be converted to predicates correctly in order to enable partition pruning.

colinmarc · 2025-07-31T19:52:02Z

The test failure seems unrelated.

Copilot

Pull Request Overview

This PR enhances DataFusion integration by adding support for timestamp ScalarValues in predicate conversion, enabling proper partition pruning for timestamp predicates. Previously, only string literals were handled correctly for date/time filtering.

Key changes:

Added timestamp scalar value handling for Second, Millisecond, Microsecond, and Nanosecond precisions
Enhanced type conversion logic in Iceberg's value system for better timestamp interoperability
Comprehensive test coverage for various timestamp formats and timezone scenarios

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

File	Description
crates/integrations/datafusion/src/physical_plan/expr_to_predicate.rs	Adds timestamp ScalarValue conversion logic and comprehensive tests for timestamp predicate handling
crates/integrations/datafusion/Cargo.toml	Adds chrono dependency for timestamp parsing functionality
crates/iceberg/src/spec/values.rs	Enhances Datum type conversion with comprehensive timestamp format support and cross-conversion capabilities

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

emkornfield

Still ramping up on the code base but left some comments.

emkornfield · 2025-09-10T17:47:02Z

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


same comment about maybe considering parameterization. of these tests.

I gave it a shot, but these tests are a lot more complicated than the tests in values.rs so it ended up being pretty ugly and I backed it out.

colinmarc · 2025-11-04T13:51:22Z

I think I addressed all comments. Thanks for the review, and sorry about the delay!

emkornfield

LGTM, hopefully a committer can review and get it merged.

kevinjqliu

hey sorry for the late reply.

Iceberg timestamptz is normalized to UTC, i think we would need to take that into consideration when converting from scalar value to iceberg datum

kevinjqliu · 2025-11-17T04:37:27Z

+        ScalarValue::TimestampSecond(Some(v), tz) => {
+            interpret_timestamptz_micros(v.checked_mul(MICROS_PER_SECOND)?, tz.as_deref())
+        }
+        ScalarValue::TimestampMillisecond(Some(v), tz) => {
+            interpret_timestamptz_micros(v.checked_mul(MICROS_PER_MILLISECOND)?, tz.as_deref())
+        }
+        ScalarValue::TimestampMicrosecond(Some(v), tz) => {
+            interpret_timestamptz_micros(*v, tz.as_deref())
+        }
+        ScalarValue::TimestampNanosecond(Some(v), Some(_)) => Some(Datum::timestamptz_nanos(*v)),
+        ScalarValue::TimestampNanosecond(Some(v), None) => Some(Datum::timestamp_nanos(*v)),


Iceberg treats timestamp with and without timestamp as 2 distinct primitive types.
https://iceberg.apache.org/spec/#primitive-types

without timezone should map to Iceberg timestamp
with timezone should map to Iceberg timestamptz

furthermore Iceberg timestamptz is represented in UTC. So when converting scalar timestamp value to Iceberg timestamptz, we would have to normalize to UTC first

Hi @kevinjqliu I think this code captures that logic? Note that the scalar value in Datafustion captures an epoch offset and an optional timezone. The Some(_) matches on the line above and returns timestamp_tz (no normalization is needed because the offset is from the epoch).

I agree interpret_timestamptz_micros might not be consistent or correct here as it is actually doing normalization (which I don't think it should if the values are coming from Arrow. Actually, looking more closely, I think the method is just doing extra work, because it first normalizes to the timezone and then denormalized again vis Datum::timestamptz_from_datetime?

I think this code captures that logic? Note that the scalar value in Datafustion captures an epoch offset and an optional timezone. The Some(_) matches on the line above and returns timestamp_tz (no normalization is needed because the offset is from the epoch).

Correct; Datafusion has the same internal representation, where the storage has an optional timezone but the timestamp is UTC. Internal to iceberg-rust there's no way to pass on the timestamp information currently (all the constructors just take the UTC stamp).

Actually, looking more closely, I think the method is just doing extra work

Yeah, it was roundtripping unecessarily. Fixed.

frisbeeman · 2025-12-29T12:07:55Z

+        ScalarValue::TimestampMicrosecond(Some(v), tz) => {
+            interpret_timestamptz_micros(*v, tz.as_deref())
+        }
+        ScalarValue::TimestampNanosecond(Some(v), Some(_)) => Some(Datum::timestamptz_nanos(*v)),


I tried to implement my own support in https://github.com/apache/iceberg-rust/pull/1968/changes#diff-979a60140760d9ab19f04b181fd2053834472f4f5c456624c85462b19430dfc0R232 and ignoring of the case if tz is not equal UTC is more valid solution. Because if we will ignore it only tz we'll select wrong files from catalog.

Hm is that really more valid? The result should be the same, and the scalar value did have a timezone, even if it was UTC.

Interesting, do you think v fromScalarValue::TimestampMicrosecond(Some(v), tz) is already adjusted for tz and contains clean UTC time?

I found this test case https://github.com/apache/datafusion/blob/83ed19235b700a2bd41283301cfd344cb5b565bc/datafusion/sql/src/unparser/expr.rs#L3265-L3270

( Arc::clone(&default_dialect), ScalarValue::TimestampMillisecond( Some(1757934000123), Some("+01:00".into()), ), "CAST('2025-09-15 12:00:00.123 +01:00' AS TIMESTAMP)", ),

Based on this data v contains a value for UTC and tz can be safely omitted.

DataFusion sometimes passes dates as string literals, but can also pass timestamp ScalarValues, which need to be converted to predicates correctly in order to enable partition pruning. This also adds support for converting date values, which helps with predicate expressions such as `date > DATE_TRUNC('day', ts)`.

- scalar_value_to_datum now distinguishes timezone presence: TimestampNanosecond with tz -> Datum::timestamptz_nanos TimestampNanosecond without tz -> Datum::timestamp_nanos Same for Microsecond/Second/Millisecond variants - Datum::to() now supports TimestampNs/TimestamptzNs conversions via convert_int/convert_long helpers (Date<->Timestamp, cross- precision, and tz<->non-tz conversions) - Fixes: "Can't convert datum from timestamp_ns to timestamptz_ns" Based on apache#1569 (not yet merged upstream). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions · 2026-02-27T00:24:15Z

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

- scalar_value_to_datum now distinguishes timezone presence: TimestampNanosecond with tz -> Datum::timestamptz_nanos TimestampNanosecond without tz -> Datum::timestamp_nanos Same for Microsecond/Second/Millisecond variants - Datum::to() now supports TimestampNs/TimestamptzNs conversions via convert_int/convert_long helpers (Date<->Timestamp, cross- precision, and tz<->non-tz conversions) - Fixes: "Can't convert datum from timestamp_ns to timestamptz_ns" Based on apache#1569 (not yet merged upstream). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

github-actions · 2026-03-06T00:27:32Z

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.

colinmarc force-pushed the df-timestamps branch 3 times, most recently from 4c291c7 to 0a16745 Compare July 31, 2025 13:40

colinmarc force-pushed the df-timestamps branch 5 times, most recently from 7629743 to ed85a34 Compare August 1, 2025 21:27

colinmarc force-pushed the df-timestamps branch from ed85a34 to 2a1f575 Compare September 10, 2025 09:08

kevinjqliu requested a review from Copilot September 10, 2025 15:48

Copilot AI reviewed Sep 10, 2025

View reviewed changes

Comment thread crates/integrations/datafusion/src/physical_plan/expr_to_predicate.rs Resolved

Comment thread crates/iceberg/src/spec/values.rs Resolved

emkornfield reviewed Sep 10, 2025

View reviewed changes

Comment thread crates/iceberg/src/spec/values.rs Resolved

emkornfield reviewed Sep 10, 2025

View reviewed changes

Comment thread crates/iceberg/src/spec/values.rs Resolved

emkornfield reviewed Sep 10, 2025

View reviewed changes

colinmarc force-pushed the df-timestamps branch from 2a1f575 to 0a0f60c Compare November 4, 2025 13:33

colinmarc force-pushed the df-timestamps branch 2 times, most recently from 30a6db5 to 86fac9a Compare November 4, 2025 14:24

emkornfield reviewed Nov 13, 2025

View reviewed changes

Comment thread crates/iceberg/src/spec/values.rs Resolved

emkornfield approved these changes Nov 13, 2025

View reviewed changes

kevinjqliu reviewed Nov 17, 2025

View reviewed changes

colinmarc force-pushed the df-timestamps branch 2 times, most recently from 6cb54ef to b43db4f Compare December 29, 2025 09:21

frisbeeman reviewed Dec 29, 2025

View reviewed changes

colinmarc force-pushed the df-timestamps branch from b43db4f to 736f2c5 Compare December 29, 2025 12:42

frisbeeman approved these changes Dec 29, 2025

View reviewed changes

frisbeeman mentioned this pull request Dec 29, 2025

(feat): Add support for timestamp datafusion pushdown #1968

Closed

colinmarc force-pushed the df-timestamps branch from 736f2c5 to c4a8c69 Compare January 7, 2026 16:46

colinmarc force-pushed the df-timestamps branch from c4a8c69 to ba8982a Compare January 12, 2026 10:28

colinmarc mentioned this pull request Feb 14, 2026

feat(datafusion): Add Timestamp scalar value conversion for predicate pushdown #2069

Merged

github-actions Bot added the stale label Feb 27, 2026

github-actions Bot closed this Mar 6, 2026

colinmarc mentioned this pull request Oct 29, 2026

fix: handle more types of timestamp conversion #3574

Open

Conversation

colinmarc commented Jul 31, 2025

Uh oh!

colinmarc commented Jul 31, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emkornfield left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emkornfield Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

colinmarc Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

colinmarc commented Nov 4, 2025

Uh oh!

Uh oh!

emkornfield left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

emkornfield Nov 18, 2025 • 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!

colinmarc Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

frisbeeman Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

colinmarc Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

frisbeeman Dec 29, 2025 • 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!

frisbeeman Dec 29, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Feb 27, 2026

Uh oh!

github-actions Bot commented Mar 6, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

emkornfield Nov 18, 2025 •

edited

Loading

frisbeeman Dec 29, 2025 •

edited

Loading