test: make expression benchmark harness fair and reproducible - #5371
Merged
Conversation
`runExpressionBenchmark` backs 27 benchmark suites. Four defects made its output untrustworthy. Apply `spark.sql.optimizer.excludedRules` to both arms rather than the Comet arm only, and append to the existing value instead of overwriting it. Excluding `ConstantFolding` for Comet alone let Spark fold expressions over literal arguments away entirely: for `select space(2) from parquetV1Table` the Spark plan was `Project [ AS space(2)]`, doing no per-row work, so that row reported a Comet regression that does not exist. Warn when the Spark baseline plan does no work beyond scanning and projecting bare attributes or literals, which means the optimizer removed the benchmarked expression. Any other node counts as work, so the join and aggregate suites are unaffected. Route both that warning and the existing not-fully-native warning to `output`, so they land in the results file above the affected table rather than scrolling past on the console. Running the suites with this in place shows `translate` in `CometStringExpressionBenchmark` and eight `c_short` cases in `CometCastNumericToNumericBenchmark` falling back to a JVM `Project`, meaning those rows labelled "Comet" were measuring Spark. Generate the base table from a pure function of the row id instead of an unseeded `scala.util.Random`, so runs are comparable. Seeding a driver-side `Random` would not have worked: the closure runs per row on the executor. Part of apache#5363.
Write warnings through `Benchmark.out`, which already tees the console and the results file, instead of hand-rolling the tee. This also keeps the warning ordered against the results table, since both now go through the same stream. Collapse `isTrivialPlan` to a single negated `exists` over the plan, dropping the mid-method `return`, the second traversal and the one-use `unwrapAlias`. Drop the untimed `noop()` from the Spark baseline check. Before execution `stripAQEPlan` already yields the initial physical plan, which carries the projections the check inspects; the nodes missing pre-execution are the codegen and columnar-transition wrappers the check classifies as no-work anyway. The Comet check keeps its execution, because AQE can re-plan a join after the first stage completes. Use `ConstantFolding.ruleName` rather than a hardcoded class name, and `Utils.stringToSeq` rather than an open-coded comma-list parser.
sunchao
approved these changes
Aug 15, 2026
sunchao
left a comment
sunchao
left a comment
Member
There was a problem hiding this comment.
Thanks for improving the benchmark harness. The deterministic inputs and persisted fallback warnings are helpful. I left one [P2] comment about making the existing space(2) case measure comparable work.
| // Constant folding is excluded so that expressions over literal arguments are still evaluated | ||
| // per row. It must be excluded for both arms: if only Comet excludes it, Spark folds the | ||
| // expression away and does no per-row work, and the comparison is meaningless. | ||
| val noConstantFolding = |
Member
There was a problem hiding this comment.
[P2] Could we make this benchmark depend on an input column rather than just disable ConstantFolding? The existing SELECT space(2) FROM parquetV1Table still compares different amounts of work: Spark's StringSpace calls UTF8String.blankString(2) once per row, while Comet's DataFusion space UDF receives ColumnarValue::Scalar, evaluates it once per batch, and broadcasts the result. The structural plan check also accepts the retained ProjectExec, so it does not flag the mismatch. Materializing an integer column and benchmarking space(column) would make both engines evaluate the expression across the rows.
Member
Author
There was a problem hiding this comment.
Fixed. Thanks for suggesting that.
Excluding `ConstantFolding` on both arms made the `space(2)` plans symmetric but not the work. Given a literal, Comet's native `space` receives a `ColumnarValue::Scalar`, builds one string per batch and lets DataFusion broadcast it, while Spark's `StringSpace` calls `UTF8String.blankString` once per row. Benchmark `space(c2)` over a new non-negative integer column instead, so both engines evaluate the expression on every row. The plan check could not catch this, because the retained `ProjectExec` looks like real work either way. Add a second check for projections that reference no input column: their value is the same for every row, so an engine is free to evaluate them once per batch, and engines differ on whether they do. Nondeterministic expressions are excluded, since `rand()` also references no column but genuinely evaluates per row. Verified that the old query trips the new warning, that the new one does not, and that `floor(rand() * 100)` does not.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Part of #5363. This covers checklist items 3, 4, 8 and 9 of that epic; the epic stays open.
Rationale for this change
runExpressionBenchmarkinCometBenchmarkBasebacks 27 benchmark suites. Four defects make its output untrustworthy, and they are worth fixing before the larger harness rework in #5363 so that later results can be compared against something sound.Constant folding was excluded for the Comet arm only.
cometExecConfigssetspark.sql.optimizer.excludedRulestoConstantFolding; the Spark case ran with folding enabled. Forselect space(2) from parquetV1TableinCometStringExpressionBenchmark, the Spark plan was:a folded literal doing no per-row work, while Comet evaluated
space(2)for every row. That row reported a Comet regression that does not exist. The conf was also assigned rather than appended, clobbering any pre-existing exclusions.Nothing checked that the benchmarked expression survived optimization. Rules that are not excluded, such as
SimplifyCastson a no-op cast, can remove the expression entirely and leave a case that measures only the scan.The not-fully-native warning went to
println.Benchmark(output = output)writes the results table to the.txt, but the warning only reached the console, so a reader of a results table cannot tell that a row labelled "Comet" actually ran on Spark.Table data came from an unseeded
scala.util.Random, so runs are not comparable. This is not only cosmetic:CometStringExpressionBenchmarkderivesREPEAT(CAST(value AS STRING), 10)from it, solpad(c1, 150, 'x')is sometimes padding and sometimes truncating.What changes are included in this PR?
All changes are in
CometBenchmarkBase.scala. No call sites change.spark.sql.optimizer.excludedRulesis now built once and applied to both arms, appending to the existing value rather than overwriting it.outputas well as the console, so they land in the results file directly above the affected table.Random.nextLong(). Seeding a driver-sideRandomwould not have worked, since the closure runs per row on the executor. The helper lives in a companion object because referencing a trait method from a Dataset closure capturesthis, which is not serializable.Two things reviewers may want to weigh in on:
Results files are not regenerated here. The final item of #5363 regenerates them once, after the baseline-case and aggregate-sink work lands.
How are these changes tested?
Benchmarks do not run in CI, so this was verified by running them locally. Compiles clean under the
spark-4.1,spark-3.5andspark-3.4profiles.SELECT sum(hash(value)), count(*)over the generated table returnedsum=-293787312920 count=65536on three independent runs across JVM restarts.space(2)fix. With folding excluded on both arms the Spark plan becomes*(1) Project [space(2) AS space(2)#1367], evaluated per row, rather than the folded literal above.spark.comet.exec.project.enabled=false. Both warnings appeared in the generated.txtabove the correct table, with the offending plan. A control case usingabs(c1)produced no warning. The temporary suite was removed.CometStringExpressionBenchmark(31 expressions),CometCastNumericToNumericBenchmarkandCometPredicateExpressionBenchmarkwere run in full. None produced a trivial-plan warning.Running the suites with the warnings visible surfaced fallbacks that were previously console-only:
translateinCometStringExpressionBenchmarkfalls back to a JVMProject.c_shortcases inCometCastNumericToNumericBenchmarkdo the same, for exampleProject [cast(c_short#19 as int) AS c_short#528].Those rows have been reporting Spark timings under a "Comet" label. I will file a separate issue rather than address them here.
Two corrections to the epic text that came out of this work, noted for whoever picks up the remaining items:
spark/benchmarksis listed in.gitignore, so no Scala microbenchmark results are tracked in git; the only committed benchmark results are the TPC JSON files underbenchmarks/results/. The.txtfiles are local artifacts pasted into PRs and issues by hand, which is where a console-only warning does its damage.Inpredicate inCometPredicateExpressionBenchmark. It does not, and cannot: Spark retainsFilterExecabove the Parquet scan even when the filter is pushed down, so that plan contains real work. Confirmed by running the suite. Moving theIninto the SELECT list remains a separate manual fix.