gabotechs commented on code in PR #94:
URL: https://github.com/apache/datafusion-iceberg/pull/94#discussion_r4443790824
##########
crates/sqllogictest/testdata/slts/df_test/timestamp_predicate_pushdown.slt:
##########
@@ -181,3 +181,53 @@ CREATE TABLE default.default.test_timestamp_millis (id INT
NOT NULL, ts TIMESTAM
# This should fail because Iceberg doesn't support second precision
statement error (?s)DataFusion error: IcebergError\(DataInvalid\).*Unsupported
Arrow data type: Timestamp\(s\)
CREATE TABLE default.default.test_timestamp_seconds (id INT NOT NULL, ts
TIMESTAMP(0))
+
+statement ok
+SET datafusion.execution.time_zone = 'UTC'
Review Comment:
Could you add a short comment on why this is needed? I'm guessing it's so
`::TIMESTAMPTZ` ends up as `+00:00`. Also it sticks around for anything added
to this file later, so maybe reset it at the end next to the DROP.
##########
crates/datafusion/src/read/predicate.rs:
##########
@@ -462,10 +462,21 @@ fn scalar_value_to_datum(value: &ScalarValue) ->
Option<Datum> {
// DataFusion's type coercion always converts them to match the column
type
// (either TimestampMicrosecond or TimestampNanosecond) before
predicate pushdown.
// See unit tests for how those conversions would work if needed.
- ScalarValue::TimestampMicrosecond(Some(v), _) => {
+ //
+ // Note: for types with TZ, the timestamp value itself is always UTC,
+ // even if the timezone is not.
Review Comment:
nit: maybe something like "Arrow stores tz-aware timestamps as UTC epoch
values, the tz string only affects display, so they map straight to Iceberg
timestamptz"? Reads a bit clearer to me. Might also be worth splitting this
from the TimestampSecond/Millisecond note above, they're kind of unrelated now.
##########
crates/sqllogictest/testdata/slts/df_test/timestamp_predicate_pushdown.slt:
##########
@@ -181,3 +181,53 @@ CREATE TABLE default.default.test_timestamp_millis (id INT
NOT NULL, ts TIMESTAM
# This should fail because Iceberg doesn't support second precision
statement error (?s)DataFusion error: IcebergError\(DataInvalid\).*Unsupported
Arrow data type: Timestamp\(s\)
CREATE TABLE default.default.test_timestamp_seconds (id INT NOT NULL, ts
TIMESTAMP(0))
+
+statement ok
+SET datafusion.execution.time_zone = 'UTC'
+
+statement ok
+CREATE TABLE default.default.test_timestamptz (id INT NOT NULL, ts_us
TIMESTAMP(6) WITH TIME ZONE, ts_ns TIMESTAMP WITH TIME ZONE)
+
+query I
+INSERT INTO default.default.test_timestamptz VALUES
+ (1, '2023-01-04T12:00:00+00:00'::TIMESTAMPTZ,
'2023-01-04T12:00:00+00:00'::TIMESTAMPTZ),
+ (2, '2023-01-06T12:00:00+00:00'::TIMESTAMPTZ,
'2023-01-06T12:00:00+00:00'::TIMESTAMPTZ)
+----
+2
+
+query TT
+EXPLAIN SELECT id FROM default.default.test_timestamptz WHERE ts_us >
'2023-01-05T00:00:00+00:00'::TIMESTAMPTZ
Review Comment:
Can we also cover the mixed cases? e.g. `ts_us > CAST('2023-01-05' AS
TIMESTAMP)` (tz column vs naive literal), and a naive `TIMESTAMP` column vs a
`::TIMESTAMPTZ` literal. Once the upstream conversion is gone a mismatched
datum would fail to bind and I think the query would just error, so it'd be
good to lock in that coercion either gives us a matching literal or skips
pushdown.
--
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]
