[SPARK-39319][CORE][SQL] Make query contexts as a part of `SparkThrowable` #37209

MaxGekk · 2022-07-17T17:11:43Z

What changes were proposed in this pull request?

In the PR, I propose to add new interface QueryContext Spark core, and allow to get an instance of QueryContext from Spark's exceptions of the type SparkThrowable. For instance, QueryContext should help users to figure out where an error occur while executing queries in Spark SQL.

Also this PR adds SqlQueryContext as one of implementation of QueryContext to Spark SQL Origin which contains a context of TreeNodes + textual summary of the error. The context value in Origin will have all necessary structural info about the fragment of SQL query to which an error can be linked.

All Spark's exceptions are modified to accept the optional QueryContext and pre-built text summary. Apparently, SQL expressions init and pass new context to exceptions.

Closes #36702

Why are the changes needed?

In the future, this enriches the information of the error message. With the change, it is possible to have a new pretty printing format error message like

> SELECT * FROM v1;

{
  “errorClass” : [ “DIVIDE_BY_ZERO” ],
  “parameters” : [ { 
                   “name” = “config”, 
                   “value” = “spark.sql.ansi.enabled” 
                 }
                 ],
  “sqlState” : “42000”,
  “context” : {
      “objectType” : “VIEW”,
      “objectName” : “default.v1”
      “indexStart” : 36,
      “indexEnd” : 41,
      “fragment” : “1 / 0” }
   }
}

Does this PR introduce any user-facing change?

Yes. The PR changes Spark's exception by replacing the type of queryContext from String to Option[QueryContext]. User's code can fail if it uses queryContext.

How was this patch tested?

By running the modified test suites:

$ build/sbt "test:testOnly *DecimalExpressionSuite"
$ build/sbt "test:testOnly *TreeNodeSuite"

and affected test suites:

$ build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite"

Authored-by: Max Gekk max.gekk@gmail.com
Co-authored-by: Gengliang Wang gengliang@apache.org

…arkthrowable

MaxGekk · 2022-07-19T16:34:57Z

@gengliangwang @cloud-fan Could you review this PR, please.

gengliangwang · 2022-07-21T08:05:19Z

@MaxGekk Sorry I was focusing on other works today. I will review it tomorrow. Thanks for taking it over!

cloud-fan · 2022-07-21T08:23:58Z

core/src/main/scala/org/apache/spark/ErrorInfo.scala

  def getMessage(
      errorClass: String,
      errorSubClass: String,
      messageParameters: Array[String],
-      queryContext: String = ""): String = {
+      queryContext: Option[QueryContext]): String = {


I think we just need a string parameter here.

I created an interface which extends QueryContext and has an additional method to get textual summary.

cloud-fan · 2022-07-21T08:27:19Z

core/src/main/scala/org/apache/spark/SparkException.scala

@@ -119,7 +119,7 @@ private[spark] class SparkArithmeticException(
    errorClass: String,
    errorSubClass: Option[String] = None,
    messageParameters: Array[String],
-    queryContext: String = "")
+    queryContext: Option[QueryContext] = None)


We should pass both QueryContext and summary: String

I think it would be better to pass one thing. I created some kind of enriched QueryContext which can give us textual summary.

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/SqlQueryContext.scala

Co-authored-by: Gengliang Wang gengliang@apache.org

…arkthrowable

MaxGekk · 2022-07-23T06:38:20Z

@gengliangwang @cloud-fan Could you review the PR, please. I addressed your comments. The test failures are not related to the changes, it seems.

…arkthrowable

cloud-fan · 2022-07-25T10:00:12Z

core/src/main/java/org/apache/spark/QueryContext.java

+public interface QueryContext {
+    // The object type of the query which throws the exception.
+    // If the exception is directly from the main query, it should be an empty string.
+    // Otherwise, it should be the exact object type in upper case. For example, a "VIEW".


not related to this PR, but we should use javadoc for public APIs, which will show up in our API doc. The same to SparkThrowable. We can fix them all in a followup.

cloud-fan · 2022-07-25T10:00:49Z

core/src/main/java/org/apache/spark/QueryContextSummary.java

+ * @since 3.4.0
+ */
+@Evolving
+public interface QueryContextSummary extends QueryContext {


does this need to be a public API?

Made it as a private one.

shall we use private[spark]? to make sure it won't show up in the API doc

This is Java. Does such syntax work here?

Since it's for internal use only, we can write it in Scala.

+1, I don't think we need this interface. Having SQLQueryContext is enough.

Let me try to remove it ...

core/src/main/scala/org/apache/spark/SparkException.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/SqlQueryContext.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/TreeNode.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/SqlQueryContext.scala

MaxGekk · 2022-07-26T12:36:42Z

The failures below:

Build modules: sql - other tests
Run TPC-DS queries with SF=1

are not related to the changes. @cloud-fan @gengliangwang Could you have a look at the PR, please.

In the log, the failed because of:

failed with exit code 137

which means OOM. Should we bump memory for Build modules: sql - other tests. WDYT, @HyukjinKwon.

…arkthrowable

HyukjinKwon · 2022-07-26T12:45:57Z

TPC-Ds should be fixed now. For sql- other tests, we might need to retrigger it for now.

MaxGekk · 2022-07-26T12:46:48Z

TPC-Ds should be fixed now. For sql- other tests, we might need to retrigger it for now.

@HyukjinKwon I have rebased on the recent master. Will see. Thank you.

gengliangwang

LGTM.

MaxGekk · 2022-07-26T15:12:01Z

Merging to master. Thank you, @gengliangwang @cloud-fan for review.

MaxGekk added 2 commits July 17, 2022 19:27

Add QueryContext

2ddaf26

Make the compiler happy

9575a41

github-actions bot added CORE SQL labels Jul 17, 2022

MaxGekk added 16 commits July 17, 2022 22:06

queryContext -> _queryContext

51e7813

Add SqlQueryContext

48cc9eb

Re-gen sql.out

758ecde

Context for DIVIDE_BY_ZERO

a6ab09f

Context for arithmetic overflow

2dc083a

Context for CANNOT_CHANGE_DECIMAL_PRECISION

c421925

Context for INVALID_ARRAY_INDEX

0b26f54

Context for MAP_KEY_DOES_NOT_EXIST

d4431f6

Context for ARITHMETIC_OVERFLOW

cfc1ad2

Context for CAST_INVALID_INPUT of boolean

d77a7f6

Context for CAST_INVALID_INPUT

508cea4

Remove _context

29e9832

Remove _context from TreeNode

27df456

Add comments

ee98642

Merge remote-tracking branch 'origin/master' into query-context-in-sp…

694c272

…arkthrowable

Fix DecimalDivideWithOverflowCheck

c6d3a5a

MaxGekk changed the title ~~[WIP][SQL] Make query contexts as a part of SparkThrowable~~ [WIP][SPARK-39319][SQL] Make query contexts as a part of SparkThrowable Jul 19, 2022

MaxGekk changed the title ~~[WIP][SPARK-39319][SQL] Make query contexts as a part of SparkThrowable~~ [WIP][SPARK-39319][CORE][SQL] Make query contexts as a part of SparkThrowable Jul 19, 2022

MaxGekk changed the title ~~[WIP][SPARK-39319][CORE][SQL] Make query contexts as a part of SparkThrowable~~ [SPARK-39319][CORE][SQL] Make query contexts as a part of SparkThrowable Jul 19, 2022

MaxGekk marked this pull request as ready for review July 19, 2022 15:37

MaxGekk requested review from gengliangwang and cloud-fan July 19, 2022 16:34

cloud-fan reviewed Jul 21, 2022

View reviewed changes

gengliangwang reviewed Jul 22, 2022

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/trees/TreeNodeSuite.scala Outdated Show resolved Hide resolved

Split QueryContext

18f767f

MaxGekk commented Jul 22, 2022

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/trees/SqlQueryContext.scala Outdated Show resolved Hide resolved

MaxGekk added 5 commits July 22, 2022 13:39

Use org.apache.spark._

d02cdeb

Check all context fields in TreeNodeSuite

4d74636

Co-authored-by: Gengliang Wang gengliang@apache.org

Revert error context code

a93e2d4

Merge remote-tracking branch 'origin/master' into query-context-in-sp…

d1f9276

…arkthrowable

Merge remote-tracking branch 'origin/master' into query-context-in-sp…

59a284e

…arkthrowable

Merge remote-tracking branch 'origin/master' into query-context-in-sp…

9159969

…arkthrowable