New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP][SPARK-39319][SQL] Make query context as part of SparkThrowable #36702
[WIP][SPARK-39319][SQL] Make query context as part of SparkThrowable #36702
Conversation
@@ -42,6 +42,8 @@ default String getSqlState() { | |||
return SparkThrowableHelper.getSqlState(this.getErrorClass()); | |||
} | |||
|
|||
default QueryContext getQueryContext() { return null; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should add some test cases for this. Will do it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we return QueryContext[]
here? The error context can be stacked, e.g. view referencing other views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, it requires efforts to refactor TreeNode.origin as Seq[Origin]
from Origin
. But let's make it an array in the interface.
// The index starts from 0. | ||
int stopIndex(); | ||
|
||
// The corresponding fragment of the SQL query which throws the exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's mention that it can be truncated if too long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I will do it in a follow-up with test cases https://issues.apache.org/jira/browse/SPARK-39365
@Evolving | ||
public interface QueryContext { | ||
// The object type of the SQL query which throws the exception. | ||
// For example, it can be empty, or a "VIEW", or a SQL "FUNCTION". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we use null or empty to represent the main query?
@@ -52,6 +53,13 @@ import org.apache.spark.util.collection.BitSet | |||
/** Used by [[TreeNode.getNodeNumbered]] when traversing the tree for a given number */ | |||
private class MutableInt(var i: Int) | |||
|
|||
case class SQLQueryContext( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we name it QueryContextImpl
?
/** | ||
* The SQL query context of current node. For example: | ||
* == SQL of VIEW v1(line 1, position 25) == | ||
* SELECT '' AS five, i.f1, i.f1 - int('2') AS x FROM INT4_TBL i | ||
* ^^^^^^^^^^^^^^^ | ||
*/ | ||
lazy val context: String = { | ||
def getFragment(): String = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is actually a pretty print of the fragment, but not the fragment itself.
Co-authored-by: Wenchen Fan <cloud0fan@gmail.com>
Co-authored-by: Wenchen Fan <cloud0fan@gmail.com>
FYI, I have taken this over, and working on it at the moment. |
What changes were proposed in this pull request?
This PR is to add a new method
getQueryContext
inSparkThrowable
.It also refactors the data type of
Origin.context
fromString
to a case classSQLQueryContext
.Why are the changes needed?
This enriches the information of the error message. With the change, it is possible to have a new pretty printing format error message like
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing UT