adriangb commented on code in PR #25099:
URL: https://github.com/apache/datafusion/pull/25099#discussion_r3968716542
##########
datafusion/sqllogictest/test_files/datetime/timestamps.slt:
##########
@@ -4301,7 +4301,6 @@ SELECT column1 FROM t_utc WHERE column1 <
'2024-02-01T00:00:00' AT TIME ZONE 'Am
query P
SELECT column1 FROM t_europe WHERE column1 = '2024-01-31T16:00:01' AT TIME
ZONE 'America/Los_Angeles';
----
-2024-02-01T00:00:01+01:00
Review Comment:
I verified this change on your branch. The empty result is correct, but the
PR does not give the reason, and the description says the opposite (see the
note from the Copilot review). Please put the reason in the PR description.
The arithmetic:
* `t_europe` holds three instants: `2023-12-31T23:00:01Z`,
`2024-01-31T23:00:01Z` and `2024-02-29T23:00:01Z`. The `union` test below this
one shows the same three instants.
* The literal `'2024-01-31T16:00:01' AT TIME ZONE 'America/Los_Angeles'` is
the instant `2024-02-01T00:00:01Z`. The `t_utc` test below this one shows the
same instant.
* No row of `t_europe` is equal to that instant. Thus the empty result is
correct, and the deleted row was a result of the bug.
An empty result is a weak assertion. A guard that is too strong also gives
an empty result, and this test cannot see the difference. Please keep a row
here. The instant `2024-01-31T23:00:01Z` is `15:00:01` in Los Angeles, thus
this query selects the second row:
```
query P
SELECT column1 FROM t_europe WHERE column1 = '2024-01-31T15:00:01' AT TIME
ZONE 'America/Los_Angeles';
----
2024-02-01T00:00:01+01:00
```
I ran this query two times on your branch: one time with your change, and
one time with the new guard removed. Without the guard it gives zero rows,
which is incorrect. With the guard it gives the row above. It is thus a correct
test for this bug. Please keep the empty case also.
##########
datafusion/expr-common/src/casts.rs:
##########
@@ -120,6 +120,21 @@ fn is_lossy_temporal_cast(from_type: &DataType, to_type:
&DataType) -> bool {
if is_date_type(from_type) && is_date_type(to_type) {
return false;
}
+ if let (DataType::Timestamp(_, from_tz), DataType::Timestamp(_, to_tz)) =
Review Comment:
I verified that this guard corrects the four queries in the issue, thank
you. Four points on the block:
1. Please add the new rule to the doc comment of `is_lossy_temporal_cast`.
The comment gives the rules for identity casts, date casts and
`Date32`/`Date64` casts in detail. It says nothing about timezones.
2. A cast between a naive timestamp and a timezone-aware timestamp is not
lossy. The cast is bijective: it moves the value by the offset of the timezone.
The doc comment of this function describes a different problem, which is a cast
that is many-to-one. The guard gives the correct result, but the name and the
comment now disagree with the code. Please make the reason clear at this
position.
3. The issue gives a second solution: keep the unwrap, but move the literal
by the same offset. That solution keeps the optimization. The present solution
stops the unwrap, and thus the engine loses the pushdown and the pruning for
each of these comparisons. Did you examine the second solution? If you prefer
the present solution, please add a comment that says that the guard is
intentionally conservative.
4. `unwrap()` is safe here, because `is_some() != is_some()` makes sure that
one side has a value. But a `match` on the two options is more clear, and it
removes the `unwrap()`.
##########
datafusion/expr-common/src/casts.rs:
##########
@@ -120,6 +120,21 @@ fn is_lossy_temporal_cast(from_type: &DataType, to_type:
&DataType) -> bool {
if is_date_type(from_type) && is_date_type(to_type) {
return false;
}
+ if let (DataType::Timestamp(_, from_tz), DataType::Timestamp(_, to_tz)) =
+ (from_type, to_type)
+ && from_tz.is_some() != to_tz.is_some()
+ {
+ let tz = from_tz.as_ref().or(to_tz.as_ref()).unwrap().as_ref();
+ if tz != "UTC"
+ && tz != "+00:00"
+ && tz != "-00:00"
+ && tz != "+0:00"
+ && tz != "-0:00"
+ && tz != "Z"
+ {
Review Comment:
This list is not exhaustive. `Etc/UTC` and `GMT` have an offset of zero, but
the code does not accept them, and thus the optimizer keeps the cast:
```sql
-- "UTC": the optimizer removes the cast
EXPLAIN SELECT * FROM t
WHERE arrow_cast(ts, 'Timestamp(Nanosecond, Some("UTC"))')
= arrow_cast(TIMESTAMP '2024-11-01T00:00:00', 'Timestamp(Nanosecond,
Some("UTC"))');
-- predicate: ts = 1730419200000000000
-- "Etc/UTC": the optimizer keeps the cast, although the offset is also zero
EXPLAIN SELECT * FROM t
WHERE arrow_cast(ts, 'Timestamp(Nanosecond, Some("Etc/UTC"))')
= arrow_cast(TIMESTAMP '2024-11-01T00:00:00', 'Timestamp(Nanosecond,
Some("Etc/UTC"))');
-- predicate: CAST(ts AS Timestamp(Nanosecond, Some("Etc/UTC"))) = ...
```
`GMT` gives the same result as `Etc/UTC`. The direction of the error is
safe, thus the rows stay correct. But the code loses the optimization for these
names, and a list of strings is difficult to keep correct.
`arrow::array::timezone::Tz` parses all of these names. Please parse the
timezone and test the offset instead of the list. If you keep the list, please
move it into a function with a name such as `is_zero_offset_timezone`, and give
the reason for each item.
##########
datafusion/expr-common/src/casts.rs:
##########
@@ -998,6 +1013,26 @@ mod tests {
assert!(is_lossy_temporal_cast(&ts, &DataType::Date32));
}
+ #[test]
+ fn test_is_lossy_temporal_cast_timestamp_tz() {
Review Comment:
This test examines `is_lossy_temporal_cast` alone. It does not show that a
query gives the correct rows. If a subsequent change makes
`unwrap_cast_in_comparison` drop the timezone shift again, this test stays
green, and the bug comes back without a failure.
Please add the queries from the issue to
`datafusion/sqllogictest/test_files/datetime/timestamps.slt`. They are the only
tests that show the behavior that this PR corrects:
```sql
statement ok
set datafusion.execution.time_zone = 'Asia/Singapore';
statement ok
create table t as select TIMESTAMP '2024-11-01T00:00:00' as ts;
statement ok
create table u as select '2024-10-31T16:00:00Z'::timestamptz as tstz;
# 2024-11-01 00:00 in Singapore is 2024-10-31 16:00 UTC
query I
select count(*) from t where ts::timestamptz =
'2024-10-31T16:00:00Z'::timestamptz;
----
1
query I
select count(*) from t where ts::timestamptz =
'2024-11-01T00:00:00Z'::timestamptz;
----
0
# the same rewrite occurs for an implicit coercion
query I
select count(*) from t where ts = '2024-10-31T16:00:00Z'::timestamptz;
----
1
# control: a column against a column, thus the optimizer unwraps nothing
query I
select count(*) from t, u where t.ts::timestamptz = u.tstz;
----
1
```
Please add two more cases:
* A timezone-aware column against a timezone-naive literal. This is the
opposite direction of the cast, and the guard is symmetric.
* An `explain` for a UTC session timezone, which shows that the optimizer
still removes the cast. Without this test, a subsequent guard that is too
strong can remove the optimization for all timezones, and each test above stays
green.
--
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]
