Skip to content

fix: make benchmark cases over ShortType columns exercise Comet - #6376

Merged
andygrove merged 1 commit into
apache:mainfrom
andygrove:benchmark-comet-rows-fallback
Sep 30, 2026
Merged

andygrove merged 1 commit into
apache:mainfrom
andygrove:benchmark-comet-rows-fallback

Conversation

@andygrove

Copy link
Copy Markdown
Member

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.

Comet falls back to Spark for any Parquet scan that reads a ShortType
column while spark.comet.scan.unsignedSmallIntSafetyCheck is enabled,
because Spark maps both signed INT16 and unsigned UINT_8 to ShortType.
Benchmark tables are written by Spark, where ShortType is always a
signed INT16, so the check only made Comet cases over a ShortType
column, such as the c_short casts in CometCastNumericToNumericBenchmark,
measure Spark. Disable it in the shared benchmark session.
@andygrove andygrove added the run-benchmark-check Run the benchmark compile and lint check on this pull request instead of waiting for the merge queue label Sep 29, 2026
@github-actions github-actions Bot added the bug Something isn't working label Sep 29, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

  • Prior state and problem: The unsigned-small-integer guard caused benchmarks reading Spark-written ShortType columns to measure Spark fallback execution in their Comet cases.
  • Design approach: Disable spark.comet.scan.unsignedSmallIntSafetyCheck in the shared benchmark session.
  • Correctness / compatibility analysis: Traced the affected fixtures and scan guard. Spark’s ParquetSchemaConverter in supported versions 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0 writes ShortType as signed INT16. This supports disabling the guard for these generated inputs. The production default remains enabled.
  • Key design decisions: Reusing the existing session configuration covers affected benchmark suites without adding an abstraction or per-row work. The configuration assignment occurs outside timed benchmark cases.
  • Implementation sketch: One configuration assignment and its explanatory comment, totaling five added lines in CometBenchmarkBase.scala.
  • Behavioral changes worth calling out: Eligible short-column scans can now run through Comet, enabling the associated native projections. Spark baseline cases retain Comet-disabled execution.
  • Suggested improvements: None meeting the P1/P2 reporting threshold. No introduced P1/P2 issues found within this review.

Reviewed the entire diff from 61810e2839d54241336be44de8765370e183a5e2 to a009ad544ae2b72ae3e5ded26167374d9ad1c976. The PR is not a draft. Snapshot and live discussion checks contained no existing reviews or comments.

Routed skills: review-comet-pr. No sibling skill applies to this benchmark-only configuration change.

Exact-head CI: Eight checks passed and fifteen were skipped, with no failures. Benchmark compilation and linting passed using Spark 4.0. Linux/macOS runtime suites and Spark SQL suites were skipped.

Validation limits: Source tracing, supported-version Spark source comparison and git diff --check completed. No local build or runtime benchmark was run because this checkout lacks JVM/native build artifacts and Spark Maven dependencies. The author’s reported Spark 4.1 runtime validation was not independently repeated.

@rich7420 rich7420 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andygrove
andygrove added this pull request to the merge queue Sep 30, 2026
Merged via the queue into apache:main with commit b58b2f3 Sep 30, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working run-benchmark-check Run the benchmark compile and lint check on this pull request instead of waiting for the merge queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nine expression benchmark rows labelled "Comet" are measuring Spark

3 participants