feat: [ANSI] Ansi sql error messages by parthchandra · Pull Request #3580 · apache/datafusion-comet

parthchandra · 2026-02-23T21:35:37Z

Which issue does this PR close?

Closes parts of #551
Closes #2215
Closes #3375

Rationale for this change

With Spark 4.0 (And Spark 3.5 with Ansi mode), Spark produces ansi compliant error messages that have an error code and in many cases include the original SQL query. When we encounter errors in native code, Comet throws a SparkException or CometNativeException that do not conform to the expected error reporting standard.

What changes are included in this PR?

This PR introduces a framework to report ansi compliant error messages from native code.

Summary of error propagation -

Spark-side query context serialization : For every serialized expression and aggregate expression, a unique expr_id is generated. If the expression's origin carries a QueryContext (SQL text, line, column, object name), it is extracted and attached to the protobuf. This is done for both Expr and AggExpr.
Native planner (planner.rs): The PhysicalPlanner now holds a QueryContextMap. When planning Expr and AggExpr nodes, if expr_id and query_context are present, the context is registered in the map. When creating physical expressions for Cast, CheckOverflow, ListExtract, SumDecimal, AvgDecimal, and arithmetic binary expressions, the relevant QueryContext is looked up and passed to the constructor.
Native errors : The SparkError enum is extended with new variants will all the Spark ANSI errors (from https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryExecutionErrors.scala). A new SparkErrorWithContext type wraps a SparkError with a QueryContext. All affected expression implementations look up the context and produce a SparkErrorWithContext when available.
The SparkError implementation also has new to_json() and exception_class() methods for JNI serialization.
JNI boundary (errors.rs -> CometQueryExecutionException): The throw_exception function now checks for SparkErrorWithContext or SparkError and throws CometQueryExecutionException. CometQueryExecutionException carries the entire SparkErrorWithContext as a JSON message. On the Scala side, CometExecIterator catches this exception and calls SparkErrorConverter.convertToSparkException() to convert to the appropriate Spark exception. If the JSON message contained the QueryContext, the exception will contain the query, otherwise it will not.
There are two version specific implementations -one for Spark 3.x (fallback to generic SparkException) and one for Spark 4.0 (calls the exact QueryExecutionErrors.* methods).

Notes: Not all expressions have been updated. All the expressions that failed unit tests as a result of incorrect error messages have been updated. ( Cast, CheckOverflow, ListExtract, SumDecimal, AvgDecimal, and binary arithmetic expressions). Binary arithmetic expressions are now represented as CheckedBinaryExpr which also includes the query context.
Most errors in QueryExecutionErrors are reproduced as is in the native side. However some errors like INTERVAL_ARITHMETIC_OVERFLOW have a version with a user suggestion and one without a user suggestion. In such cases there are two variants in the native side.

How are these changes tested?

New unit tests. Failing tests listed in #551, #2215, #3375

This PR was produced with the generous assistance of Claude Code

parthchandra · 2026-02-23T22:04:37Z

@coderfender, fyi

coderfender · 2026-02-23T22:15:48Z

Thank you @parthchandra . This is awesome

spark/src/test/scala/org/apache/comet/CometCastSuite.scala

spark/src/main/spark-4.0/org/apache/spark/sql/comet/shims/ShimSparkErrorConverter.scala

andygrove · 2026-02-23T22:56:34Z

native/spark-expr/src/query_context.rs

+
+        // Extract the problematic fragment
+        let fragment = if start_idx < self.sql_text.len() && stop_idx <= self.sql_text.len() {
+            &self.sql_text[start_idx..stop_idx]


The earlier docs say that start_idx is a character index, but it is being used as a byte index here, I think. Perhaps you could tests for non-ASCII cases, if that makes sense?

Good catch. Fixed to use char index. Added a unit test

parthchandra · 2026-02-24T01:36:21Z

Changed to draft to figure out backward compatibility

parthchandra · 2026-02-27T17:01:10Z

@andygrove @coderfender This is ready for review. I'll rebase after the review.

andygrove · 2026-03-02T21:57:55Z

native/org/apache/spark/sql/errors/QueryExecutionErrors.scala

+ * This does not include exceptions thrown during the eager execution of commands, which are
+ * grouped into [[QueryCompilationErrors]].
+ */
+private[sql] object QueryExecutionErrors extends QueryErrorsBase with ExecutionErrors {


This Scala class is in the native module. Should this be here?

No idea how this got there. This is a spark class! Removed.
Thanks for catching this.

parthchandra · 2026-03-02T23:22:13Z

Also fixed another failing test.

andygrove · 2026-03-03T00:00:10Z

common/src/main/java/org/apache/comet/exceptions/CometQueryExecutionException.java

+   */
+  public boolean isJsonMessage() {
+    String msg = getMessage();
+    return msg != null && msg.trim().startsWith("{") && msg.trim().endsWith("}");


nit: trim is called twice

andygrove · 2026-03-03T00:09:51Z

native/spark-expr/src/agg_funcs/avg_decimal.rs

-            if overflowed {
-                // Set to None if overflow happens
+
+            if overflowed || !is_valid_decimal_precision(result, self.sum_precision) {


This looks like a change in functionality? Why is the is_valid_decimal_precision check now needed?

This is a tricky one. overflowing_add checks for integer overflow but decimal can overflow if the precision is exceeded so we need to check for that as well. This is caught by the DataFrameAggregateSuite.checkAggResultsForDecimalOverflow test

parthchandra requested a review from andygrove February 23, 2026 21:43

parthchandra force-pushed the sql-query-errors branch from fc0b78f to cf4588f Compare February 23, 2026 22:04

andygrove reviewed Feb 23, 2026

View reviewed changes

spark/src/test/scala/org/apache/comet/CometCastSuite.scala Show resolved Hide resolved

andygrove reviewed Feb 23, 2026

View reviewed changes

spark/src/main/spark-4.0/org/apache/spark/sql/comet/shims/ShimSparkErrorConverter.scala Show resolved Hide resolved

andygrove reviewed Feb 23, 2026

View reviewed changes

parthchandra marked this pull request as draft February 24, 2026 01:02

coderfender mentioned this pull request Feb 24, 2026

[EPIC] Fully support ANSI mode #313

Open

parthchandra force-pushed the sql-query-errors branch from 71caba8 to e386824 Compare February 25, 2026 02:27

parthchandra marked this pull request as ready for review February 26, 2026 16:32

parthchandra force-pushed the sql-query-errors branch from 3ebdb5a to 9f6e462 Compare February 26, 2026 21:37

feat: [ANSI] Ansi sql error messages

8b6f25c

parthchandra force-pushed the sql-query-errors branch from 9f6e462 to 8b6f25c Compare February 26, 2026 23:58

fix after rebase

5525659

parthchandra mentioned this pull request Mar 2, 2026

feat: Cast numeric (non int) to timestamp #3559

Open

andygrove reviewed Mar 2, 2026

View reviewed changes

fixes

4ea56dc

andygrove reviewed Mar 3, 2026

View reviewed changes

trim once

0a876ef

Conversation

parthchandra commented Feb 23, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

parthchandra commented Feb 23, 2026

Uh oh!

coderfender commented Feb 23, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthchandra commented Feb 24, 2026

Uh oh!

parthchandra commented Feb 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthchandra commented Mar 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants