-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
[SPARK-36943][SQL] Improve readability of missing column error message #34202
Conversation
…seable grammar Signed-off-by: Karen Feng <karen.feng@databricks.com>
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #143897 has finished for PR 34202 at commit
|
Test build #143932 has started for PR 34202 at commit |
Test build #143958 has finished for PR 34202 at commit
|
Test build #143987 has finished for PR 34202 at commit
|
Signed-off-by: Karen Feng <karen.feng@databricks.com>
@cloud-fan and @HyukjinKwon, can you help take a look? |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #144001 has finished for PR 34202 at commit
|
@@ -169,6 +169,37 @@ trait AnalysisTest extends PlanTest { | |||
} | |||
} | |||
|
|||
protected def assertAnalysisErrorClass( |
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.
qq why does this have Class
in the end? Maybe we should just name it something like assertAnalysisErrors
vs assertAnalysisError
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.
it's testing error-class ...
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.
Yes, that's why. I thought about changing assertAnalysisError
to assertAnalysisErrorMessage
, but I think I've altered more than enough code in this PR already.
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.
ohhhh gotya!
private[spark] def orderStringsBySimilarity( | ||
baseString: String, | ||
testStrings: Seq[String]): Seq[String] = { | ||
testStrings.sortWith { (a, b) => |
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.
nit: testStrings.sortBy(ACLStringUtils.getLevenshteinDistance(_, baseString))
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.
Done, thanks @cloud-fan!
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #144035 has finished for PR 34202 at commit
|
thanks, merging to master! |
What changes were proposed in this pull request?
Improves the quality of the error message encountered by users when they attempt to access a column that does not exist.
Removes the lingo term "resolve" and sorts the suggestions by probability.
Why are the changes needed?
Improves the user experience
Does this PR introduce any user-facing change?
Yes:
Before:
After:
How was this patch tested?
Unit tests