andygrove opened a new pull request, #6868:
URL: https://github.com/apache/datafusion-comet/pull/6868
## Which issue does this PR close?
There is no issue for this. It follows up the timezone audit (#6335), whose
label bugs, such as #6328 and #6330, shipped with passing tests.
## Rationale for this change
Native plans label every `TimestampType` value `Timestamp(µs, "UTC")` and
every `TimestampNTZType` value with no timezone. The label never changes the
values, so `checkSparkAnswer` and friends cannot see a wrong one: they compare
collected rows, `export_batch` hands each array to the JVM with whatever type
the plan produced, and `CometPlainVector.getLong` reads the raw microseconds.
The casts at `ScanExec` and in the shuffle writer's `SchemaAlignExec` repair a
wrong label as well.
Inside a native plan the label does matter. Another zone breaks comparisons
and `CASE`/`coalesce` coercion against `"UTC"` arrays, and a missing one makes
kernels such as `hour` and casts to string read the value as wall-clock time.
So a test of an expression's direct output passes even when the expression
breaks a query that composes it, which is how #6328 and #6330 shipped with
passing tests.
## What changes are included in this PR?
An internal, test-only config,
`spark.comet.testing.strictTimestampLabels.enabled`, which `CometTestBase`
turns on for every Comet suite. It defaults to false, so nothing changes
outside the test suites. With it on:
- `CometExecIterator` checks each batch a native plan returns against the
plan's Spark output types, including nested and dictionary-encoded columns
(`NativeUtil.checkTimestampLabels`). The callers that know their output types
pass them in a new `outputTypes` parameter: `CometNativeExec`,
`CometNativeScanExec`, `CometIcebergNativeScanExec`, the native limit, and
`CometTakeOrderedAndProjectExec`.
- `ScanExec` fails, instead of casting, on an input timestamp whose label
differs from the declared type.
- The native shuffle writer fails planning, instead of wrapping its child in
`SchemaAlignExec`, when the child labels a timestamp differently from the
declared schema. The error includes the native plan, so it points at the
expression.
The native checks share `describe_timestamp_label_mismatch` in
`datafusion-comet-common`.
These are boundary checks. A wrong label that is consumed entirely inside
one native plan, such as `hour(f(ts))` where `f` drops the label, never reaches
a boundary, and still shows up only as wrong values in a non-UTC session.
## How are these changes tested?
- Unit tests: Rust tests for `describe_timestamp_label_mismatch`, the strict
`ScanExec`, and the strict shuffle-writer alignment. `NativeUtilSuite` tests
for the JVM check, covering nested types, maps, dictionary values imported over
FFI, and closing a batch it rejects. `CometExecSuite`'s config serde test
covers the new natively read key.
- Planted bugs, to show the check fires: with `timestamp_trunc` changed back
to labelling its result with the session timezone (its behavior before #6345),
`SELECT c0, date_trunc('HOUR', c0) FROM tbl` and a `GROUP BY` over the
truncated value both passed `checkSparkAnswer` with the check off. With it on,
they failed at the plan output and at the shuffle writer. With
`CometSparkToColumnarExec` labelling its timestamps `Etc/UTC`, the `ScanExec`
check failed the same way. On unmodified code the same probes raised nothing.
- With the check on, 2,446 tests in 26 timestamp-heavy suites pass on Spark
4.1, so `main` has no label drift on those paths. They cover the SQL file
suite, casts, Parquet reads, expressions, joins, aggregates, windows, shuffle,
the in-memory cache, fuzz, the Python runner, and the Iceberg read and write
suites.
--
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]
