[SPARK-34234][SQL] Remove TreeNodeException that didn't work #31337
+96
−175
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #134476 has finished for PR 31337 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #134501 has finished for PR 31337 at commit
|
Contributor
Author
|
cc @cloud-fan |
cloud-fan
reviewed
Feb 5, 2021
| a | ||
| } else { | ||
| BoundReference(ordinal, a.dataType, input(ordinal).nullable) | ||
| sys.error(s"Couldn't find $a in ${input.attrs.mkString("[", ",", "]")}") |
Contributor
There was a problem hiding this comment.
since we are here, let's change it to throw new IllegalStateException, as sys.error exists the JVM.
cloud-fan
reviewed
Feb 5, 2021
| * Thrown when an invalid attempt is made to access a property of a tree that has yet to be fully | ||
| * resolved. | ||
| */ | ||
| class UnresolvedException[TreeType <: TreeNode[_]](tree: TreeType, function: String) |
Contributor
There was a problem hiding this comment.
Seems we don't need the tree parameter
Contributor
Member
|
yea, removing it looks fine. |
viirya
reviewed
Feb 5, 2021
| |${sideBySide(plan.treeString, reOptimized.treeString).mkString("\n")} | ||
| """.stripMargin | ||
| throw new TreeNodeException(reOptimized, message, null) | ||
| throw new AnalysisException(message) |
Member
There was a problem hiding this comment.
Is this always an analysis exception? Seems to me it is not only for analyzer rule.
Member
There was a problem hiding this comment.
For a place like this, should we keep tree node exception?
Contributor
Author
There was a problem hiding this comment.
TreeNodeException makes stacktrace verbose and complicated. Yeah! it was work for analyzer, optimizer and expressions, do we need to create RuleExeception? or you have another idea.
Contributor
There was a problem hiding this comment.
RuntimeException is also fine. It's test-only and most likely people don't care about exception type here.
viirya
reviewed
Feb 5, 2021
| } | ||
| } | ||
|
|
||
| protected override def doExecute(): RDD[InternalRow] = attachTree(this, "execute") { |
Member
There was a problem hiding this comment.
Removing tree node exception for a place like this looks good to me. The error happened here should not be node tree related.
|
Test build #134975 has finished for PR 31337 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #134976 has finished for PR 31337 at commit
|
|
Kubernetes integration test status success |
Contributor
Author
|
retest this please |
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
|
Test build #135062 has finished for PR 31337 at commit
|
|
Kubernetes integration test starting |
|
Test build #135065 has finished for PR 31337 at commit
|
|
Kubernetes integration test status success |
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
Test build #135068 has finished for PR 31337 at commit
|
Contributor
Author
|
cc @cloud-fan |
maropu
reviewed
Feb 10, 2021
| /** | ||
| * Functions for attaching and retrieving trees that are associated with errors. | ||
| */ | ||
| package object errors { |
maropu
approved these changes
Feb 10, 2021
cloud-fan
reviewed
Feb 10, 2021
| a | ||
| } else { | ||
| BoundReference(ordinal, a.dataType, input(ordinal).nullable) | ||
| throw QueryExecutionErrors.cannotFindExpressionInInputAttributesError(a, input) |
Contributor
There was a problem hiding this comment.
nit: previously it's sys.error, which means it's something that shouldn't hit. I think it's also assert-like and we can just use IllegalStateException here.
Contributor
Author
There was a problem hiding this comment.
Yeah. It hears more reasonable.
cloud-fan
approved these changes
Feb 10, 2021
|
Kubernetes integration test starting |
|
Kubernetes integration test status success |
Contributor
|
thanks, merging to master! |
Contributor
Author
|
@cloud-fan Thanks for your work! @maropu @viirya Thanks for your review! |
|
Test build #135088 has finished for PR 31337 at commit
|
|
Test build #135089 has finished for PR 31337 at commit
|
MaxGekk
reviewed
Dec 5, 2023
Comment on lines
+79
to
+80
| throw new IllegalStateException( | ||
| s"Couldn't find $a in ${input.attrs.mkString("[", ",", "]")}") |
Member
There was a problem hiding this comment.
Is it supposed an internal error, correct? or end users might face to the error on some SQL queries?
Contributor
Author
There was a problem hiding this comment.
I guess we should use internal error too. At that time, we didn't have this understanding yet.
Member
|
@beliefer @cloud-fan I understand that the intention of this change is to eliminate redundant messages for internal errors that occur during the SparkPlan execution. But I think it makes the error message less useful if something goes wrong in the analyze or optimize phases. For example, I hit an error that happens in the rule the below stacktrace comes from the internal version based on OSS Spark 3.3 |
Contributor
Author
|
It seems the |
Member
@beliefer, absolutely right, but which node? I expect a sub-plan tree to be printed to narrow the scope from a huge whole plan tree. |
Contributor
|
so |
Contributor
Author
|
Got it. I have no idea if it is worth to keep. I think we can print more info for |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
TreeNodeExceptioncauses the error msg not clear and it didn't work well.Because the
TreeNodeExceptionlooks redundancy, we could remove it.There are show a case:
The above code will use
HashAggregateExec. In order to ensure that an exception will be thrown when executingHashAggregateExec, I addedthrow new RuntimeException("calculate error")intospark/sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/HashAggregateExec.scala
Line 85 in 72b7f8a
So, if the above code is executed,
RuntimeException("calculate error")will be thrown.Before this PR, the error is:
After this PR, the error is:
Why are the changes needed?
TreeNodeExceptiondidn't work well.Does this PR introduce any user-facing change?
'No'.
How was this patch tested?
Jenkins test.