feat: [ANSI] Ansi sql error messages#3580
Conversation
parthchandra
force-pushed
the
sql-query-errors
branch
from
fc0b78f to
cf4588f
Compare
|
@coderfender, fyi |
|
Thank you @parthchandra . This is awesome |
| // context eagerly so it displays the call site at the | ||
| // line of code where the cast method was called, whereas spark grabs the context | ||
| // lazily and displays the call site at the line of code where the error is checked. | ||
| assert(sparkMessage.startsWith(cometMessage.substring(0, 40))) |
There was a problem hiding this comment.
is 40 just an arbitrary number to get a decent sized chunk of the error message or does it have a specific meaning in the context of this test?
There was a problem hiding this comment.
I used 40 as an arbitrary number to get enough of the error message. There is no special significance.
| case "BinaryArithmeticOverflow" => | ||
| Some( | ||
| QueryExecutionErrors.binaryArithmeticCauseOverflowError( | ||
| params("value1").toString.toShort, |
There was a problem hiding this comment.
Could this overflow if the value is larger than a short?
There was a problem hiding this comment.
I believe that should not happen. The original Spark exception has these values declared as short.
|
|
||
| // 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] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Good catch. Fixed to use char index. Added a unit test
|
Changed to draft to figure out backward compatibility |
parthchandra
force-pushed
the
sql-query-errors
branch
from
71caba8 to
e386824
Compare
3 participants
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 -
expr_idis generated. If the expression's origin carries aQueryContext(SQL text, line, column, object name), it is extracted and attached to the protobuf. This is done for bothExprandAggExpr.QueryContextMap. When planningExprandAggExprnodes, ifexpr_idandquery_contextare 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.SparkErrorenum 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 newSparkErrorWithContexttype wraps aSparkErrorwith aQueryContext. All affected expression implementations look up the context and produce aSparkErrorWithContextwhen available.The
SparkErrorimplementation also has newto_json()andexception_class()methods for JNI serialization.throw_exceptionfunction now checks forSparkErrorWithContextorSparkErrorand throwsCometQueryExecutionException.CometQueryExecutionExceptioncarries the entireSparkErrorWithContextas a JSON message. On the Scala side,CometExecIteratorcatches this exception and callsSparkErrorConverter.convertToSparkException()to convert to the appropriate Spark exception. If the JSON message contained theQueryContext, the exception will contain the query, otherwise it will not.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
CheckedBinaryExprwhich also includes the query context.Most errors in
QueryExecutionErrorsare reproduced as is in the native side. However some errors likeINTERVAL_ARITHMETIC_OVERFLOWhave 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