Skip to content

[SPARK-29482][SQL] ANALYZE TABLE should look up catalog/table like v2 commands #26129

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.

Sign up for GitHub

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

Jump to bottom
Closed
cloud-fan wants to merge 3 commits into from
Closed

[SPARK-29482][SQL] ANALYZE TABLE should look up catalog/table like v2 commands #26129

cloud-fan wants to merge 3 commits into from

Conversation

@cloud-fan
Copy link
Contributor

@cloud-fan cloud-fan commented Oct 15, 2019 •
edited
Loading

What changes were proposed in this pull request?

Add AnalyzeTableStatement and AnalyzeColumnStatement, and make ANALYZE TABLE go through the same catalog/table resolution framework of v2 commands.

Why are the changes needed?

It's important to make all the commands have the same table resolution behavior, to avoid confusing end-users. e.g.

USE my_catalog
DESC t // success and describe the table t from my_catalog
ANALYZE TABLE t // report table not found as there is no table t in the session catalog

Does this PR introduce any user-facing change?

yes. When running ANALYZE TABLE, Spark fails the command if the current catalog is set to a v2 catalog, or the table name specified a v2 catalog.

How was this patch tested?

new tests

@cloud-fan
Copy link
Contributor Author

cloud-fan commented Oct 15, 2019

cc @imback82 @rdblue @dongjoon-hyun @HyukjinKwon

@SparkQA
Copy link

SparkQA commented Oct 15, 2019

Test build #112118 has finished for PR 26129 at commit 601cf66.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class AnalyzeTableStatement(
  • case class AnalyzeColumnStatement(

imback82
imback82 reviewed Oct 15, 2019
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated
* Create an [[AnalyzeTableStatement]], or an [[AnalyzeColumnStatement]].
* Example SQL for analyzing a table or a set of partitions :
* {{{
* ANALYZE TABLE [db_name.]tablename [PARTITION (partcol1[=val1], partcol2[=val2], ...)]
Copy link
Contributor

@imback82 imback82 Oct 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: db_name. needs to be updated now that multipartIdentifier is parsed? (although the scenario is not currently supported)

imback82
imback82 approved these changes Oct 15, 2019
View reviewed changes
Copy link
Contributor

@imback82 imback82 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (one minor comment)

@SparkQA
Copy link

SparkQA commented Oct 16, 2019

Test build #112136 has finished for PR 26129 at commit df18d7a.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor Author

cloud-fan commented Oct 16, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Oct 16, 2019

Test build #112164 has finished for PR 26129 at commit df18d7a.

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

gengliangwang
gengliangwang reviewed Oct 17, 2019
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala Outdated
case class UseStatement(isNamespaceSet: Boolean, nameParts: Seq[String]) extends ParsedStatement

/**
* A ANALYZE TABLE statement, as parsed from SQL.
Copy link
Member

@gengliangwang gengliangwang Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: A => An

gengliangwang
gengliangwang reviewed Oct 17, 2019
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/statements.scala Outdated
noScan: Boolean) extends ParsedStatement

/**
* A ANALYZE TABLE FOR COLUMNS statement, as parsed from SQL.
Copy link
Member

@gengliangwang gengliangwang Oct 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

gengliangwang
gengliangwang approved these changes Oct 17, 2019
View reviewed changes
Copy link
Member

@gengliangwang gengliangwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except minor comments

@SparkQA
Copy link

SparkQA commented Oct 18, 2019

Test build #112255 has finished for PR 26129 at commit 78e1495.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya
Copy link
Member

viirya commented Oct 18, 2019

retest this please.

@SparkQA
Copy link

SparkQA commented Oct 18, 2019

Test build #112259 has finished for PR 26129 at commit 78e1495.

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

@gengliangwang
Copy link
Member

gengliangwang commented Oct 18, 2019

Thanks, merging to master!

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

Reviewers

@viirya viirya viirya approved these changes

@gengliangwang gengliangwang gengliangwang approved these changes

+1 more reviewer

@imback82 imback82 imback82 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@cloud-fan @SparkQA @viirya @gengliangwang @imback82 @dongjoon-hyun