[SPARK-34475][SQL] Rename logical nodes of v2 `ALTER` commands #31596

MaxGekk · 2021-02-19T20:07:30Z

What changes were proposed in this pull request?

In the PR, I propose to rename logical nodes of v2 commands in the form: <verb> + <object> like:

AlterTableAddPartition -> AddPartition
AlterTableSetLocation -> SetTableLocation

Why are the changes needed?

For simplicity and readability of logical plans
For consistency with other logical nodes. For example, the logical node RenameTable for ALTER TABLE .. RENAME TO was added before AlterTableRenamePartition.

Does this PR introduce any user-facing change?

Should not since this is non-public APIs.

How was this patch tested?

Check scala style: ./dev/scalastyle
Affected test suites:

$ build/sbt -Phive-2.3 -Phive-thriftserver "test:testOnly *AlterTableRenamePartitionSuite"

MaxGekk · 2021-02-19T20:11:38Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala

 * }}}
 */
-case class AlterTableAddPartition(
+case class AddPartition(


Maybe AddPartitions because the command can add multiple partitions.

imback82

LGTM except for pending comments including AddPartitions.

sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala

SparkQA · 2021-02-19T21:07:39Z

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

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala

MaxGekk · 2021-02-19T21:29:36Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala

 * The logical plan of the ALTER TABLE ... RENAME TO PARTITION command.
 */
-case class AlterTableRenamePartition(
+case class RenamePartition(


Even now, we allow to rename only one partition but Hive supports renaming of multiple partitions in one command. Maybe Spark will support this too in the future. Should we name it as RenamePartitions?

yea, to be consistent with ADD and DROP

MaxGekk · 2021-02-19T21:33:02Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala

 * The logical plan of the ALTER TABLE ... SET LOCATION command.
 */
-case class AlterTableSetLocation(
+case class SetTableLocation(


The command incapsulates 2 commands actually: set table location and set partition location. Should we split it to SetTableLocation and SetPartitionLocation (separately in another PR).

SparkQA · 2021-02-19T21:42:50Z

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

SparkQA · 2021-02-20T01:06:12Z

Test build #135277 has finished for PR 31596 at commit 5109748.

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

SparkQA · 2021-02-20T15:11:36Z

Test build #135312 has finished for PR 31596 at commit 6da1d08.

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

dongjoon-hyun · 2021-02-21T17:22:37Z

cc @cloud-fan

MaxGekk · 2021-02-22T05:29:26Z

@HyukjinKwon @cloud-fan @dongjoon-hyun Can this be merged to branch-3.1? Most (or all) of the nodes were added after 3.0, and they haven't released yet.

SparkQA · 2021-02-22T07:13:27Z

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

SparkQA · 2021-02-22T07:41:27Z

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

cloud-fan · 2021-02-22T08:37:39Z

RC3 is cut and I don't think we should do this refactor in 3.1.2 (assuming RC3 passes).

SparkQA · 2021-02-22T09:01:13Z

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

SparkQA · 2021-02-22T09:30:59Z

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

SparkQA · 2021-02-22T11:15:59Z

Test build #135334 has finished for PR 31596 at commit 09e7268.

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

SparkQA · 2021-02-22T12:39:44Z

Test build #135339 has finished for PR 31596 at commit f6ab9ab.

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

HyukjinKwon · 2021-02-22T13:03:35Z

RC3 is cut and I don't think we should do this refactor in 3.1.2 (assuming RC3 passes).

@cloud-fan, hm are there any API change here?

cloud-fan · 2021-02-22T13:22:42Z

no, logical plan is not a public API.

cloud-fan · 2021-02-23T12:04:29Z

thanks, merging to master!

MaxGekk added 12 commits February 19, 2021 22:11

AlterTableAddPartition -> AddPartition

4388358

AlterTableDropPartition -> DropPartition

b4b6c15

AlterTableRenamePartition -> RenamePartition

91b738f

AlterNamespaceSetProperties -> SetNamespaceProperties

2a50be1

AlterNamespaceSetLocation -> SetNamespaceLocation

f26ac6f

AlterViewSetProperties -> SetViewProperties

c3938a6

AlterViewUnsetProperties -> UnsetViewProperties

19dbddd

AlterTableSerDeProperties -> SetTableSerDeProperties

1280b32

AlterTableSetLocation -> SetTableLocation

905f873

AlterTableSetProperties -> SetTableProperties

f7b6ce2

AlterTableUnsetProperties -> UnsetTableProperties

d314b60

Fix imports

5109748

github-actions bot added the SQL label Feb 19, 2021

MaxGekk mentioned this pull request Feb 19, 2021

[SPARK-34465][SQL] Rename v2 alter table exec nodes #31584

Closed

MaxGekk commented Feb 19, 2021

View reviewed changes

MaxGekk changed the title ~~[SPARK-34475][SQL] Rename logical nodes of v2 commands~~ [SPARK-34475][SQL] Rename logical nodes of v2 ALTER commands Feb 19, 2021

imback82 reviewed Feb 19, 2021

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala Outdated Show resolved Hide resolved

MaxGekk commented Feb 19, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2Commands.scala Outdated Show resolved Hide resolved

MaxGekk commented Feb 19, 2021

View reviewed changes

MaxGekk added 3 commits February 20, 2021 13:51

Address review comments

2a721ad

AddPartition -> AddPartitions

8270b6e

DropPartition -> DropPartitions

6da1d08

cloud-fan approved these changes Feb 22, 2021

View reviewed changes

RenamePartition -> RenamePartitions

09e7268

Trigger build

f6ab9ab

HyukjinKwon approved these changes Feb 23, 2021

View reviewed changes

cloud-fan closed this in 8f994cb Feb 23, 2021

[SPARK-34475][SQL] Rename logical nodes of v2 ALTER commands #31596

[SPARK-34475][SQL] Rename logical nodes of v2 ALTER commands #31596

Uh oh!

Conversation

MaxGekk commented Feb 19, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

MaxGekk Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

imback82 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Feb 19, 2021

Uh oh!

Uh oh!

MaxGekk Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

cloud-fan Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

MaxGekk Feb 19, 2021

Choose a reason for hiding this comment

Uh oh!

cloud-fan Feb 22, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Feb 19, 2021

Uh oh!

SparkQA commented Feb 20, 2021

Uh oh!

SparkQA commented Feb 20, 2021

Uh oh!

dongjoon-hyun commented Feb 21, 2021

Uh oh!

MaxGekk commented Feb 22, 2021

Uh oh!

SparkQA commented Feb 22, 2021

Uh oh!

SparkQA commented Feb 22, 2021

Uh oh!

cloud-fan commented Feb 22, 2021

Uh oh!

SparkQA commented Feb 22, 2021

Uh oh!

SparkQA commented Feb 22, 2021

Uh oh!

SparkQA commented Feb 22, 2021

Uh oh!

SparkQA commented Feb 22, 2021

Uh oh!

HyukjinKwon commented Feb 22, 2021

Uh oh!

cloud-fan commented Feb 22, 2021

Uh oh!

cloud-fan commented Feb 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[SPARK-34475][SQL] Rename logical nodes of v2 `ALTER` commands #31596

[SPARK-34475][SQL] Rename logical nodes of v2 `ALTER` commands #31596