apache / apache/datafusion-comet

Nine expression benchmark rows labelled "Comet" are measuring Spark

Open
#5,372 0 comments 0 reactions 1 assignee Claimed by @andygrove View on GitHub
area:expressions bug performance priority:low test
Dominant language
Scala
Stars
1.3k
Forks
373
Avg merge
2d 4h
Merged PRs (30d)
198

Description

## Describe the bug

Nine rows across two expression benchmark suites are labelled `Comet` but are measuring Spark. In each case the plan falls back to a JVM `Project` sitting on top of `CometColumnarToRow`, so the expression itself never runs natively.

`runExpressionBenchmark` has always checked for this, but it reported the result with `println`, so the warning went to the console while the results table went to the `.txt`. Anyone reading the table, or a copy of it pasted into a PR, sees a `Comet` row with no indication that it ran on Spark. #5371 routes that warning into the results file, which is how these surfaced.

## Affected rows

`CometStringExpressionBenchmark` (1 row):

```
*(1) Project [translate(c1#12, 123456, aBcDeF) AS translate(c1, 123456, aBcDeF)#23815]
+- *(1) CometColumnarToRow
+- CometNativeScan parquet [c1#12] ...
```

`CometCastNumericToNumericBenchmark` (8 rows), all casts from `ShortType`:

```
Project [cast(c_short#19 as int) AS c_short#528]
Project [cast(c_short#19 as bigint) AS c_short#831L]
Project [cast(c_short#19 as tinyint) AS c_short#3765]
Project [cast(c_short#19 as float) AS c_short#5321]
Project [try_cast(c_short#19 as int) AS c_short#1904]
Project [try_cast(c_short#19 as bigint) AS c_short#2218L]
Project [try_cast(c_short#19 as tinyint) AS c_short#4308]
Project [try_cast(c_short#19 as float) AS c_short#6690]
```

The short cases are the surprising ones: Comet supports short-to-int and short-to-long casts, and the equivalent casts from other numeric types in the same suite do run natively. Something specific to `ShortType` in this suite is preventing the projection from being replaced. Worth understanding before assuming the benchmark is simply exercising an unsupported path.

## Steps to reproduce

With #5371 applied:

```sh
SPARK_GENERATE_BENCHMARK_FILES=1 make benchmark-org.apache.spark.sql.benchmark.CometCastNumericToNumericBenchmark
SPARK_GENERATE_BENCHMARK_FILES=1 make benchmark-org.apache.spark.sql.benchmark.CometStringExpressionBenchmark
```

Then grep the generated files under `spark/benchmarks/` for `First non-Comet operator`.

Without #5371, the same warnings appear on stdout during the run.

## Expected behavior

Either the expressions run natively, or the benchmark is adjusted so the row is not presented as a Comet measurement.

## Additional context

Found while working on #5363. The fallback is the bug here; the reporting gap that hid it is fixed separately in #5371.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.