Conversation
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: The README and benchmark page displayed Comet 1.0.0 results measured against Spark 3.5.8.
- Design approach: Add versioned benchmark data and charts using the existing generator. This introduces no runtime overhead or new abstraction.
- Correctness / compatibility analysis: Both JSON files contain the same 103 queries with valid positive timings. Recomputed totals are 966.3875s for Spark and 524.7405s for Comet, giving 1.84165× speedup and 45.70% less elapsed time, consistent with the rounded headline. Spark 4.2 remains identified as experimental in the README and compatibility documentation. No Spark execution semantics change.
- Key design decisions: Preserve the previous results and document the new Spark/Comet versions and two-iteration mean methodology.
- Implementation sketch: Add two JSON files and four PNGs, then update the README and TPC-DS page references. All referenced images exist, and all four charts were inspected alongside the generator.
- Behavioral changes worth calling out: The headline now compares against Spark 4.2. The charts retain the four queries where Comet is slower.
- Suggested improvements: No introduced P1/P2 issues found within this review.
Reviewed the full eight-file diff from base 76493618769ff85e7a43e6f6c79f8b58d9e0b1ad to head 62d3a1726f757738c51f5d8686d231f2071f6c12. Confirmed the PR is not a draft. The supplied discussion snapshot contains no reviews, issue comments, inline comments, or threads.
Routed skills: review-comet-pr. No sibling skill applies to this documentation-only change.
Exact-head CI: Required Checks, Preflight, CodeQL, and other executed checks passed. Runtime suites and site deployment were skipped. Preflight includes successful Markdown formatting and license checks.
Validation limits: Local JSON integrity, query coverage, arithmetic, image-reference, PNG integrity, and git diff --check checks passed. No local documentation build or Spark/native suites were run. The EKS benchmark, individual iteration timings, and cluster configuration were not independently reproduced.
Which issue does this PR close?
N/A
Rationale for this change
The README and the TPC-DS benchmark page show 1.0.0 results measured on Spark 3.5.8. This updates them for Comet 1.1.0, which is about to be released, measured on Spark 4.2.0.
What changes are included in this PR?
benchmarks/results/1.1.0/(spark-tpcds.json,comet-tpcds.json), in the same format as the 1.0.0 ones.docs/source/_static/images/benchmark-results/1.1.0/, generated withbenchmarks/tpc/generate-comparison.py.benchmark-results/tpc-ds.mdnow use the 1.1.0 charts. The TPC-DS page now states the Spark and Comet versions and that each query was run twice (mean reported).Across the 103 queries, the geometric mean speedup is 1.66x. Comet is slower than Spark on 4 queries, most notably q54 (also slower in 1.0.0) and q68 (1.4s in 1.0.0, 3.9s in 1.1.0).
Spark 4.2 support is still experimental. I used it as the headline because it's the newest Spark version, but I'm happy to switch the headline comparison to 4.1 if people prefer.
How are these changes tested?
The benchmarks ran TPC-DS SF1000 (Parquet) on the same EKS setup as the 1.0.0 results (
r6i.24xlarge, data in S3), with the configuration documented on the page:branch-1.1(ee3f239).I checked from each run's Spark environment that both used the documented settings, and that Comet was fully disabled in the Spark run.
prettier --checkpasses on the changed Markdown.