sunchao commented on code in PR #5371:
URL: https://github.com/apache/datafusion-comet/pull/5371#discussion_r3790307994
##########
spark/src/test/scala/org/apache/spark/sql/benchmark/CometBenchmarkBase.scala:
##########
@@ -125,50 +131,103 @@ trait CometBenchmarkBase
extraCometConfigs: Map[String, String] = Map.empty): Unit = {
val benchmark = new Benchmark(name, cardinality, output = output)
- benchmark.addCase("Spark") { _ =>
- withSQLConf(CometConf.COMET_ENABLED.key -> "false") {
- spark.sql(query).noop()
- }
- }
+ // 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 =
Review 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.
--
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]
