andygrove opened a new pull request, #6376:
URL: https://github.com/apache/datafusion-comet/pull/6376
## Which issue does this PR close?
Closes #5372.
## Rationale for this change
The issue lists nine expression benchmark rows labelled `Comet` that run on
Spark. On current main they split into two groups.
The eight `ShortType` casts in `CometCastNumericToNumericBenchmark` (`CAST`
and `TRY_CAST` of `c_short` to `INT`, `LONG`, `BYTE` and `FLOAT`) still fall
back, and the cause is the scan rather than the cast or the projection.
`CometScanRule.isTypeSupported` rejects any Parquet scan that reads a
`ShortType` column while `spark.comet.scan.unsignedSmallIntSafetyCheck` is true
(the default), because Spark maps both signed `INT16` and unsigned `UINT_8` to
`ShortType` and Comet cannot tell them apart from the schema. The scan stays
Spark's `FileScan parquet`, so the `Project` above it is not converted either.
The extended explain for `SELECT CAST(c_short AS INT) FROM parquetV1Table`
shows:
```
Project
+- ColumnarToRow
+- Scan parquet [COMET: Unsupported schema
StructType(StructField(c_short,ShortType,true)): Native Parquet scan may not
handle unsigned UINT_8 correctly for ShortType. Set
spark.comet.scan.unsignedSmallIntSafetyCheck=false to allow native execution if
your data does not contain unsigned small integers. ...]
```
That is why only the `c_short` cases were affected: every other source
column in the suite is read by a native scan, and the short-to-numeric casts
themselves are `Compatible`. The fallback is intended for arbitrary Parquet
files, but benchmark tables are written by Spark, where `ShortType` is always a
signed `INT16`, so the check only turns these Comet cases into Spark
measurements. `CometTestBase` disables the check for the same reason, and #5718
disabled it for `CometColumnarToRowBenchmark`.
The `translate` row in `CometStringExpressionBenchmark` no longer falls
back. Since #5032, `CometStringTranslate` goes through the codegen dispatcher
by default, so the plan is `CometProject` over `CometNativeScan` and
`runExpressionBenchmark` reports no warning. It needs no change here.
The same check also affects other suites that read a `ShortType` column
through the shared benchmark session: `CAST(c_short AS BOOLEAN)` in
`CometCastBooleanBenchmark`, `CAST(c_short AS STRING)` in
`CometCastNumericToStringBenchmark`, `hash(c_short)` in
`CometHashExpressionBenchmark`, and the `SMALLINT` column scan in
`CometReadBenchmark`.
## What changes are included in this PR?
- `CometBenchmarkBase.getSparkSession` sets
`spark.comet.scan.unsignedSmallIntSafetyCheck=false` next to the other session
defaults, with a comment explaining why this is safe for benchmark tables.
Doing it in the shared session covers every suite that uses it, not only
`CometCastNumericToNumericBenchmark`.
## How are these changes tested?
This only changes benchmark configuration, so there is no new test. To check
it, I reproduced the benchmark setup in a throwaway suite (not committed) that
extends `CometBenchmarkBase`, builds the same tables as
`CometCastNumericToNumericBenchmark` and `CometStringExpressionBenchmark`, and
applies the same check as `runExpressionBenchmark` (run the query with Comet
enabled, strip AQE, call `findFirstNonCometOperator`), plus the extended
explain. I ran it on the default Spark 4.1 profile with a debug native build.
- Before the change, the first non-Comet operator for all eight `c_short`
casts was `Project`, with the scan fallback reason shown above. The other 60
cast cases and the 31 non-collated string cases, including `translate`, were
fully Comet.
- After the change, all 68 cast cases are fully Comet (`CometProject` over
`CometNativeScan`). Calling `runExpressionBenchmark` itself for the eight
`c_short` cases and `translate` produces no "NOT fully Comet native" warning.
The `c_short` queries from the other suites listed above fall back when the
check is forced on and are fully Comet with the new session default.
`./mvnw -B -DskipTests test-compile` (with scalastyle) and the syntactic
scalafix check pass.
--
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]
