Skip to content

Conversation

@imback82
Copy link
Contributor

What changes were proposed in this pull request?

Based on the discussion #30743 (comment), this PR proposes to remove the command name in AnalysisException message when a relation is not resolved.

For some of the commands that use UnresolvedTable, UnresolvedView, and UnresolvedTableOrView to resolve an identifier, when the identifier cannot be resolved, the exception will be something like Table or view not found for 'SHOW TBLPROPERTIES': badtable. The command name (SHOW TBLPROPERTIES in this case) should be dropped to be consistent with other existing commands.

Why are the changes needed?

To make the exception message consistent.

Does this PR introduce any user-facing change?

Yes, the exception message will be changed from

Table or view not found for 'SHOW TBLPROPERTIES': badtable

to

Table or view not found: badtable

for commands that use UnresolvedTable, UnresolvedView, and UnresolvedTableOrView to resolve an identifier.

How was this patch tested?

Updated existing tests.

@github-actions github-actions bot added the SQL label Dec 16, 2020
@SparkQA
Copy link

SparkQA commented Dec 16, 2020

Test build #132866 has finished for PR 30794 at commit 610c677.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Dec 16, 2020

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37478/

@SparkQA
Copy link

SparkQA commented Dec 16, 2020

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37478/

@SparkQA
Copy link

SparkQA commented Dec 16, 2020

Test build #132877 has finished for PR 30794 at commit 73081c1.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@imback82
Copy link
Contributor Author

cc @cloud-fan

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 8666d1c Dec 16, 2020
@imback82
Copy link
Contributor Author

imback82 commented Dec 16, 2020

@cloud-fan Should we also remove command name from the following:

def expectTableNotTempViewError(quoted: String, cmd: String, t: TreeNode[_]): Throwable = {
new AnalysisException(s"$quoted is a temp view. '$cmd' expects a table",
t.origin.line, t.origin.startPosition)
}
def expectTableOrPermanentViewNotTempViewError(
quoted: String, cmd: String, t: TreeNode[_]): Throwable = {
new AnalysisException(s"$quoted is a temp view. '$cmd' expects a table or permanent view.",
t.origin.line, t.origin.startPosition)

We can do s"$quoted is a temp view, not a table" and s"$quoted is a temp view, not a table or permanent view." respectively. And we can remove commandName from UnresolvedTable|View|TableOrView. WDYT?

@cloud-fan
Copy link
Contributor

Since we don't have a clear rule about when a command expects table only and when a command expects table or view, I think it still makes sense to clarify it in the error message, e.g. '$cmd' expects a table or permanent view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants