Repository navigation
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem:
expect_errorchecks message substrings and permits Spark fallback, so it cannot establish Comet error-class parity. - Design approach: Add
expect_error_classusing the existingcheckSparkErrorhelper, preserving legacy behavior. - Correctness / compatibility analysis: The helper checks planned Comet operators, exact error class, exception type, SQLSTATE and cause chains. Spark source checks across 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0 confirm the APIs and fixture error classes. No introduced P1/P2 issues found within this review.
- Key design decisions: Reusing the helper avoids duplicate parity logic and version branches. Changes are confined to tests and contributor documentation, adding no production execution overhead.
- Implementation sketch: Add the assertion mode, validate its directive syntax, dispatch to the helper, accept it as a codegen sentinel, and cover parser and runner behavior.
- Behavioral changes worth calling out: Malformed directives fail with source locations. Operator checks inspect the initial plan under AQE. Query-context assertions remain in Scala. The touched existing files on latest release branch
branch-1.1match the PR base, so release-relative changes are the same intended framework additions. - Suggested improvements: None meeting the P1/P2 reporting threshold.
Reviewed both commits and all five changed files at 1aa87d1a7c97e41bff54c9173da3b4fa2015c654 against 93ce5c3471dbd89aa5cd5f6f9e26df27adab6a8a. Applied review-comet-pr; no sibling subsystem skill applies. Confirmed non-draft status. Existing reviews, issue comments, inline comments and review threads were empty.
Exact-head CI: 23 checks succeeded, 14 were skipped, and the Spark 4.1 expressions job remained in progress. No failures were reported. Spark SQL, Iceberg and macOS jobs were skipped.
Validation: All 16 Spark-independent parser tests passed in a disposable Scala 2.13.17 harness. Base/head parser outputs matched for all 510 existing SQL fixtures. git diff --check passed. The unchanged fixture-gating parser test and end-to-end Spark/Comet runner tests were not executed locally. This checkout has no built native library, and a full Maven/native build was not performed. The working tree remains unchanged.
Which issue does this PR close?
Closes #6617.
Rationale for this change
Legacy
expect_errorchecks message substrings and allows Spark fallback, so it cannot establish native execution or exact error-class parity. Add a strict SQL-fixture mode that uses the existing Scala error-checking contract.What changes are included in this PR?
Add
query expect_error_class(ERROR_CLASS)usingcheckSparkErrorto assert Comet operators, exact error class, exception-class and SQLSTATE parity, and noCometNativeExceptionin either cause chain. Validate malformed directives with source locations and document the mode.The strict mode can serve as a codegen error sentinel. Legacy message-only behavior remains unchanged, and query-context assertions stay in Scala.
How are these changes tested?