Repository navigation
fix: make benchmark cases over ShortType columns exercise Comet - #6376
Conversation
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.
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: The unsigned-small-integer guard caused benchmarks reading Spark-written
ShortTypecolumns to measure Spark fallback execution in their Comet cases. - Design approach: Disable
spark.comet.scan.unsignedSmallIntSafetyCheckin the shared benchmark session. - Correctness / compatibility analysis: Traced the affected fixtures and scan guard. Spark’s
ParquetSchemaConverterin supported versions 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0 writesShortTypeas signedINT16. 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.
Which issue does this PR close?
Closes #5372.
Rationale for this change
The issue lists nine expression benchmark rows labelled
Cometthat run on Spark. On current main they split into two groups.The eight
ShortTypecasts inCometCastNumericToNumericBenchmark(CASTandTRY_CASTofc_shorttoINT,LONG,BYTEandFLOAT) still fall back, and the cause is the scan rather than the cast or the projection.CometScanRule.isTypeSupportedrejects any Parquet scan that reads aShortTypecolumn whilespark.comet.scan.unsignedSmallIntSafetyCheckis true (the default), because Spark maps both signedINT16and unsignedUINT_8toShortTypeand Comet cannot tell them apart from the schema. The scan stays Spark'sFileScan parquet, so theProjectabove it is not converted either. The extended explain forSELECT CAST(c_short AS INT) FROM parquetV1Tableshows:That is why only the
c_shortcases were affected: every other source column in the suite is read by a native scan, and the short-to-numeric casts themselves areCompatible. The fallback is intended for arbitrary Parquet files, but benchmark tables are written by Spark, whereShortTypeis always a signedINT16, so the check only turns these Comet cases into Spark measurements.CometTestBasedisables the check for the same reason, and #5718 disabled it forCometColumnarToRowBenchmark.The
translaterow inCometStringExpressionBenchmarkno longer falls back. Since #5032,CometStringTranslategoes through the codegen dispatcher by default, so the plan isCometProjectoverCometNativeScanandrunExpressionBenchmarkreports no warning. It needs no change here.The same check also affects other suites that read a
ShortTypecolumn through the shared benchmark session:CAST(c_short AS BOOLEAN)inCometCastBooleanBenchmark,CAST(c_short AS STRING)inCometCastNumericToStringBenchmark,hash(c_short)inCometHashExpressionBenchmark, and theSMALLINTcolumn scan inCometReadBenchmark.What changes are included in this PR?
CometBenchmarkBase.getSparkSessionsetsspark.comet.scan.unsignedSmallIntSafetyCheck=falsenext 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 onlyCometCastNumericToNumericBenchmark.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 asCometCastNumericToNumericBenchmarkandCometStringExpressionBenchmark, and applies the same check asrunExpressionBenchmark(run the query with Comet enabled, strip AQE, callfindFirstNonCometOperator), plus the extended explain. I ran it on the default Spark 4.1 profile with a debug native build.c_shortcasts wasProject, with the scan fallback reason shown above. The other 60 cast cases and the 31 non-collated string cases, includingtranslate, were fully Comet.CometProjectoverCometNativeScan). CallingrunExpressionBenchmarkitself for the eightc_shortcases andtranslateproduces no "NOT fully Comet native" warning. Thec_shortqueries 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.