andygrove opened a new issue, #6617: URL: https://github.com/apache/datafusion-comet/issues/6617
Part of #6615. ### What is the problem the feature request solves? `query expect_error(<substr>)` passes when Spark and Comet both throw and both messages contain `<substr>`. It does not check that Comet ran the query. If the operator falls back to Spark, Spark raises the same error and the query still passes. Today only one convention guards against this: fixtures that turn on the codegen dispatcher must also contain a plain `query` sentinel (`requireSentinelForCodegenExpectError`). There are 226 `expect_error` queries in 59 fixtures. None of them checks native execution directly, so a serde change that makes an ANSI error path fall back would go unnoticed. The Scala suites use `checkSparkError(df, errorClass)` for the same purpose, and it is much stronger. It asserts that: - the plan is fully Comet; - Spark's error class is the expected one; - Comet throws the same exception class, error class and SQLSTATE; - Comet's cause chain contains no `CometNativeException`. Some Scala tests go further and compare the query context, i.e. `getQueryContext` / the fragment of the expression that failed. Examples are the #2215 and #3375 regressions, the decimal divide/cast overflow tests, and the `make_date` exception test. Error parity is the point of these Scala tests, so they cannot move to SQL fixtures without losing coverage. The audit for #6615 counted about 23 of them: - `CometExpressionSuite`: ANSI add/subtract/multiply/divide, decimal divide by zero, decimal cast overflow, decimal multiply overflow, `hll_union_agg` lgConfigK. - `CometAggregateSuite`: ANSI decimal SUM overflow at maximum precision. - `CometNativeCastSuite`: the cast error message test (#2215). - `CometArrayExpressionSuite`: `get_array_item` / `element_at` errors (#3375). - `CometMapExpressionSuite`: `map_from_arrays` with unequal lengths. - `CometTemporalExpressionSuite`: `make_date` / `next_day` exceptions. - `CometRegExpJvmSuite`: invalid patterns and `regexp_replace` errors. There are more in the window, codegen and smaller suites. ### Describe the potential solution Add a query mode that runs the same checks as `checkSparkError`. One possible shape: ```sql -- asserts native plan, error class, SQLSTATE and exception-class parity, no CometNativeException query expect_error_class(ARITHMETIC_OVERFLOW) SELECT a + b FROM t ``` Optionally add query-context parity, so the error points at the same fragment in both engines. Then decide what plain `expect_error` should mean. Either it also asserts native execution, with an explicit opt-out for errors raised on a fallback path, or the existing queries move to the new mode wherever they pass. Converting the 226 queries is not required for #6615. It would close the same hole for the fixtures that already exist. Related, from the same audit: `expect_error(ARITHMETIC_OVERFLOW)` also matches `BINARY_ARITHMETIC_OVERFLOW`, so it cannot catch the error-class divergence that the gated Spark 4.2 Scala tests target (#4967). An error-class equality check fixes that too. -- 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]
