adriangb commented on PR #25164:
URL: https://github.com/apache/datafusion/pull/25164#issuecomment-5636344925
**Self-review (QA pass) of our own PR, at head `2b0a37fb91`.** I tried to
break the main claim of this file: "each query is a real agreement between
DataFusion and PostgreSQL".
**Result.** The file passes against `postgres:15`, the image that CI uses. I
found one query where a naive projection hides a real divergence. I found no
other serious problem.
## Findings (most important first)
### 1. Lines 112-118: a `::timestamp` cast hides a real divergence
```sql
SELECT (TIMESTAMP '2024-01-15 12:00:00' AT TIME ZONE
'America/Denver')::timestamp
= (TIMESTAMP '2024-07-01 12:00:00' AT TIME ZONE
'America/Denver')::timestamp - INTERVAL '168 days'
```
- The query returns a `boolean`. So the harness does not need a naive
projection here.
- The two casts move `- INTERVAL '168 days'` onto naive values. A naive
value has no DST, so the query cannot see a DST difference.
- Without the casts, the engines disagree in Block A:
| Engine | Session time zone | Without the casts | With the casts (the file)
|
| --- | --- | --- | --- |
| PostgreSQL 15 | `UTC` | `f` | `f` |
| DuckDB 1.5.2 | `UTC` | `false` | not measured |
| DataFusion (`datafusion-cli` at the base of this PR) | `+00:00` | `true` |
`false` |
| PostgreSQL 15 | `America/Denver` | `t` | not measured |
| DataFusion | `America/Denver` | `true` | not measured |
- DataFusion applies a day interval in the time zone of the value (Denver).
PostgreSQL applies it in the session time zone (UTC).
- https://github.com/apache/datafusion/pull/25175 pins this divergence at
lines 1027-1038.
- The comment above the query says that it tests DST offsets. With the
casts, a change to DST behaviour cannot make this query fail.
Recommendation: remove the two casts and move the query to Block B. In Block
B both engines use `America/Denver`, and both return `true`. If you keep the
query in Block A, add a comment that names the divergence.
### 2. Lines 452-460: the comment describes the opposite case
The comment says: "the same UTC instant reads as a different local hour".
Rows 3 and 4 are two different instants (`19:00Z` in January, `18:00Z` in
July). Both read as local hour `12`. The query is valid, but the comment is
wrong.
### 3. `::bigint` removes the fractional part of `epoch`
The two engines convert a floating-point value to `bigint` in different ways:
| Expression | DataFusion | PostgreSQL 15 |
| --- | --- | --- |
| `1.6` to `bigint` | `1` | `2` |
| `-1.6` to `bigint` | `-1` | `-2` |
DataFusion truncates and PostgreSQL rounds. Each value in the file is a
whole second, so no query gives a different answer today. But the cast removes
any sub-second part before the comparison. So the file cannot detect a
sub-second error in `epoch`, and a future row with a fractional second can fail
for a reason unrelated to time zones. Low priority: keep the data at whole
seconds, and say so in the header.
## What I checked and found correct
- **PostgreSQL 15 run.** I ran the full `pg_compat` suite with
`PG_COMPAT=true` against a fresh `postgres:15` container. All 7 files pass.
- **Proof that PostgreSQL really executes this file.** I set `log_statement
= 'all'` and ran this file alone. The PostgreSQL log shows the harness session
execute statements `s3` to `s91`, from the first `SET TimeZone = 'UTC'` to the
final one, with zero errors.
- **DataFusion-only mode.** The file also passes without `PG_COMPAT`.
- **CI.** The "Run sqllogictest with Postgres runner" job passes at this
head.
- **Version sensitivity.** On `postgres:15`, `extract(epoch ...)` renders as
`1719792000.000000` and `date_part('epoch', ...)` renders as `1719792000`. Each
query in the file casts both to `bigint`, so the result text does not depend on
the `numeric` scale.
- **The harness constraint in the header is correct.**
`postgres_engine/mod.rs` renders `Type::TIMESTAMP` and calls `unimplemented!`
for other types, `timestamptz` included.
- **Masking audit of the other queries.**
- Block A projects to `::timestamp`. DataFusion renders an aware value in
UTC, and PostgreSQL uses the session time zone. Block A sets UTC, so these
projections agree by construction. The header states this constraint.
- Blocks B and C do not use `::timestamp`. Each result goes through
`date_part`. DataFusion reads the time zone of the value, and PostgreSQL reads
the session time zone. Each block sets the two to the same zone.
- Filters, joins, `UNION`, `CASE`, `COALESCE`, `greatest`, `least` and the
`date_bin` origins use literals with an explicit offset. So no naive literal
enters a comparison, and https://github.com/apache/datafusion/issues/25095
cannot affect them.
- No query applies `AT TIME ZONE` to an aware value. So
https://github.com/apache/datafusion/pull/25165 does not change this file.
I did not run the file against PostgreSQL 17. CI uses 15, so 15 is the
version that matters.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
