Skip to content
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

[SPARK-33514][SQL] Migrate TRUNCATE TABLE command to use UnresolvedTable to resolve the identifier #30457

Closed
wants to merge 4 commits into from
Closed

[SPARK-33514][SQL] Migrate TRUNCATE TABLE command to use UnresolvedTable to resolve the identifier #30457

wants to merge 4 commits into from

Conversation

imback82
Copy link
Contributor

What changes were proposed in this pull request?

This PR proposes to migrate TRUNCATE TABLE to use UnresolvedTable to resolve the table identifier. This allows consistent resolution rules (temp view first, etc.) to be applied for both v1/v2 commands. More info about the consistent resolution rule proposal can be found in JIRA or proposal doc.

Note that TRUNCATE TABLE works only with v1 tables, and not supported for v2 tables.

Why are the changes needed?

The changes allow consistent resolution behavior when resolving the table identifier. For example, the following is the current behavior:

sql("CREATE TEMPORARY VIEW t AS SELECT 1")
sql("CREATE DATABASE db")
sql("CREATE TABLE t using csv AS SELECT 1")
sql("USE db")
sql("TRUNCATE TABLE t") // Succeeds

With this PR, TRUNCATE TABLE above fails with the following:

org.apache.spark.sql.AnalysisException: t is a temp view not table.; line 1 pos 0
    at org.apache.spark.sql.catalyst.analysis.package$AnalysisErrorAt.failAnalysis(package.scala:42)
    at org.apache.spark.sql.catalyst.analysis.Analyzer$ResolveTempViews$$anonfun$apply$7.$anonfun$applyOrElse$42(Analyzer.scala:866)

, which is expected since temporary view is resolved first and TRUNCATE TABLE doesn't support a temporary view.

Does this PR introduce any user-facing change?

After this PR, TRUNCATE TABLE is resolved to a temp view t instead of table db.t in the above scenario.

How was this patch tested?

Updated existing tests.

@github-actions github-actions bot added the SQL label Nov 22, 2020
@SparkQA
Copy link

SparkQA commented Nov 22, 2020

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

@SparkQA
Copy link

SparkQA commented Nov 22, 2020

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

@SparkQA
Copy link

SparkQA commented Nov 22, 2020

Test build #131485 has finished for PR 30457 at commit fabd755.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class TruncateTable(

@SparkQA
Copy link

SparkQA commented Nov 22, 2020

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

@SparkQA
Copy link

SparkQA commented Nov 22, 2020

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

@SparkQA
Copy link

SparkQA commented Nov 22, 2020

Test build #131489 has finished for PR 30457 at commit f1e7130.

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

@imback82
Copy link
Contributor Author

cc @cloud-fan (GA passed)

cloud-fan
cloud-fan approved these changes Nov 23, 2020
View reviewed changes
Copy link
Contributor

@cloud-fan cloud-fan left a comment

Choose a reason for hiding this comment

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

LGTM, please fix conflicts.

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

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

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

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

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

Test build #131562 has finished for PR 30457 at commit f2473bd.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class UnresolvedTable(
  • case class CurrentTimeZone() extends LeafExpression with Unevaluable
  • trait PathFilterStrategy extends Serializable
  • trait StrategyBuilder
  • class PathGlobFilter(filePatten: String) extends PathFilterStrategy
  • abstract class ModifiedDateFilter extends PathFilterStrategy
  • class ModifiedBeforeFilter(thresholdTime: Long, val timeZoneId: String)
  • class ModifiedAfterFilter(thresholdTime: Long, val timeZoneId: String)

@SparkQA
Copy link

SparkQA commented Nov 23, 2020

Test build #131563 has finished for PR 30457 at commit 27d6f92.

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

@SparkQA
Copy link

SparkQA commented Nov 24, 2020

Test build #131589 has finished for PR 30457 at commit 27d6f92.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in fdd6c73 Nov 24, 2020
@imback82 imback82 deleted the truncate_table branch November 24, 2020 19:36
cloud-fan
cloud-fan reviewed Dec 16, 2020
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
*
* For example:
* {{{
* TRUNCATE TABLE multi_part_name [PARTITION (partcol1=val1, partcol2=val2 ...)]
* }}}
*/
override def visitTruncateTable(ctx: TruncateTableContext): LogicalPlan = withOrigin(ctx) {
TruncateTableStatement(
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't we remove TruncateTableStatement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch! Created a follow up PR: #30811

@imback82 imback82 mentioned this pull request Dec 16, 2020
Closed
dongjoon-hyun pushed a commit that referenced this pull request Dec 16, 2020
@imback82 @dongjoon-hyun
[SPARK-33514][SQL][FOLLOW-UP] Remove unused TruncateTableStatement ca…
e7e29fd 
…se class

### What changes were proposed in this pull request?

This PR removes unused `TruncateTableStatement`: #30457 (comment)

### Why are the changes needed?

To remove unused `TruncateTableStatement` from #30457.

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

Not needed.

Closes #30811 from imback82/remove_truncate_table_stmt.

Authored-by: Terry Kim <yuminkim@gmail.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
dongjoon-hyun pushed a commit that referenced this pull request Dec 16, 2020
@imback82 @dongjoon-hyun
[SPARK-33514][SQL][FOLLOW-UP] Remove unused TruncateTableStatement ca…
7269867 
…se class

This PR removes unused `TruncateTableStatement`: #30457 (comment)

To remove unused `TruncateTableStatement` from #30457.

No

Not needed.

Closes #30811 from imback82/remove_truncate_table_stmt.

Authored-by: Terry Kim <yuminkim@gmail.com>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit e7e29fd)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloud-fan cloud-fan cloud-fan approved these changes

Assignees
No one assigned
Labels
SQL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@imback82 @SparkQA @cloud-fan