Skip to content

[SPARK-36380][SQL] Simplify the logical plan names for ALTER TABLE ... COLUMN #33609

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
wants to merge 1 commit into from
Closed

[SPARK-36380][SQL] Simplify the logical plan names for ALTER TABLE ... COLUMN #33609

wants to merge 1 commit into from

Conversation

cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

This a followup of the recent work such as #33200

For ALTER TABLE commands, the logical plans do not have the common AlterTable prefix in the name and just use names like SetTableLocation. This PR proposes to follow the same naming rule in ALTER TABE ... COLUMN commands.

This PR also moves these AlterTable commands to a individual file and give them a base trait.

Why are the changes needed?

name simplification

Does this PR introduce any user-facing change?

no

How was this patch tested?

existing test

@cloud-fan
Copy link
Contributor Author

cc @imback82 @MaxGekk

@github-actions github-actions bot added the SQL label Aug 2, 2021
@SparkQA
Copy link

SparkQA commented Aug 2, 2021

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

@SparkQA
Copy link

SparkQA commented Aug 2, 2021

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

@SparkQA
Copy link

SparkQA commented Aug 2, 2021

Test build #141952 has finished for PR 33609 at commit d736cb9.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • trait AlterTableCommand extends UnaryCommand
  • case class CommentOnTable(table: LogicalPlan, comment: String) extends AlterTableCommand
  • case class SetTableLocation(
  • case class SetTableProperties(
  • case class UnsetTableProperties(
  • case class AddColumns(
  • case class ReplaceColumns(
  • case class DropColumns(
  • case class RenameColumn(
  • case class AlterColumn(

imback82
imback82 approved these changes Aug 2, 2021
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, thanks @cloud-fan!

@SparkQA
Copy link

SparkQA commented Aug 2, 2021

Test build #141951 has finished for PR 33609 at commit d0a260e.

  • This patch fails from timeout after a configured wait of 500m.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • trait AlterTableCommand extends UnaryCommand
  • case class CommentOnTable(table: LogicalPlan, comment: String) extends AlterTableCommand
  • case class SetTableLocation(
  • case class SetTableProperties(
  • case class UnsetTableProperties(
  • case class AddColumns(
  • case class ReplaceColumns(
  • case class DropColumns(
  • case class RenameColumn(
  • case class AlterColumn(

@MaxGekk
Copy link
Member

MaxGekk commented Aug 3, 2021

+1, LGTM. Merging to master and to 3.2 (as it is follow up of adding logical plans that haven't released yet).
Thank you, @cloud-fan and @imback82 for review.

@MaxGekk MaxGekk closed this in 7cb9c1c Aug 3, 2021
MaxGekk pushed a commit that referenced this pull request Aug 3, 2021
@cloud-fan @MaxGekk
[SPARK-36380][SQL] Simplify the logical plan names for ALTER TABLE ..…
7c58684 
…. COLUMN

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

This a followup of the recent work such as #33200

For `ALTER TABLE` commands, the logical plans do not have the common `AlterTable` prefix in the name and just use names like `SetTableLocation`. This PR proposes to follow the same naming rule in `ALTER TABE ... COLUMN` commands.

This PR also moves these AlterTable commands to a individual file and give them a base trait.

### Why are the changes needed?

name simplification

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

no

### How was this patch tested?

existing test

Closes #33609 from cloud-fan/dsv2.

Authored-by: Wenchen Fan <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
(cherry picked from commit 7cb9c1c)
Signed-off-by: Max Gekk <[email protected]>
@panbingkun panbingkun mentioned this pull request Aug 1, 2024
Closed
LuciferYang pushed a commit that referenced this pull request Aug 1, 2024
@panbingkun @LuciferYang
[SPARK-49076][SQL] Fix the outdated logical plan name in `AstBuilde…
abf9bac 
…r's` comments

### What changes were proposed in this pull request?
The pr aims to fix the outdated `logical plan name` in `AstBuilder's` comments.

### Why are the changes needed?
- After the pr #33609, the name of the logical plan below has been changed:
  `AlterTableAddColumns` -> `AddColumns`
  `AlterTableRenameColumn` -> `RenameColumn`
  `AlterTableAlterColumn` -> `AlterColumn`
  `AlterTableDropColumns` -> `DropColumns`

- After the pr #30398
The name of the logical plan `ShowPartitionsStatement` has been changed to `ShowPartitions`.

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

### How was this patch tested?
Only update comments.

### Was this patch authored or co-authored using generative AI tooling?
No.

Closes #47562 from panbingkun/fix_astbuilder.

Lead-authored-by: panbingkun <[email protected]>
Co-authored-by: panbingkun <[email protected]>
Signed-off-by: yangjie01 <[email protected]>
fusheng9399 pushed a commit to fusheng9399/spark that referenced this pull request Aug 6, 2024
@panbingkun
[SPARK-49076][SQL] Fix the outdated logical plan name in `AstBuilde…
d5042ed 
…r's` comments

### What changes were proposed in this pull request?
The pr aims to fix the outdated `logical plan name` in `AstBuilder's` comments.

### Why are the changes needed?
- After the pr apache#33609, the name of the logical plan below has been changed:
  `AlterTableAddColumns` -> `AddColumns`
  `AlterTableRenameColumn` -> `RenameColumn`
  `AlterTableAlterColumn` -> `AlterColumn`
  `AlterTableDropColumns` -> `DropColumns`

- After the pr apache#30398
The name of the logical plan `ShowPartitionsStatement` has been changed to `ShowPartitions`.

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

### How was this patch tested?
Only update comments.

### Was this patch authored or co-authored using generative AI tooling?
No.

Closes apache#47562 from panbingkun/fix_astbuilder.

Lead-authored-by: panbingkun <[email protected]>
Co-authored-by: panbingkun <[email protected]>
Signed-off-by: yangjie01 <[email protected]>
attilapiros pushed a commit to attilapiros/spark that referenced this pull request Oct 4, 2024
@panbingkun @attilapiros
[SPARK-49076][SQL] Fix the outdated logical plan name in `AstBuilde…
255c54e 
…r's` comments

### What changes were proposed in this pull request?
The pr aims to fix the outdated `logical plan name` in `AstBuilder's` comments.

### Why are the changes needed?
- After the pr apache#33609, the name of the logical plan below has been changed:
  `AlterTableAddColumns` -> `AddColumns`
  `AlterTableRenameColumn` -> `RenameColumn`
  `AlterTableAlterColumn` -> `AlterColumn`
  `AlterTableDropColumns` -> `DropColumns`

- After the pr apache#30398
The name of the logical plan `ShowPartitionsStatement` has been changed to `ShowPartitions`.

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

### How was this patch tested?
Only update comments.

### Was this patch authored or co-authored using generative AI tooling?
No.

Closes apache#47562 from panbingkun/fix_astbuilder.

Lead-authored-by: panbingkun <[email protected]>
Co-authored-by: panbingkun <[email protected]>
Signed-off-by: yangjie01 <[email protected]>
himadripal pushed a commit to himadripal/spark that referenced this pull request Oct 19, 2024
@panbingkun
[SPARK-49076][SQL] Fix the outdated logical plan name in `AstBuilde…
cf6c79a 
…r's` comments

### What changes were proposed in this pull request?
The pr aims to fix the outdated `logical plan name` in `AstBuilder's` comments.

### Why are the changes needed?
- After the pr apache#33609, the name of the logical plan below has been changed:
  `AlterTableAddColumns` -> `AddColumns`
  `AlterTableRenameColumn` -> `RenameColumn`
  `AlterTableAlterColumn` -> `AlterColumn`
  `AlterTableDropColumns` -> `DropColumns`

- After the pr apache#30398
The name of the logical plan `ShowPartitionsStatement` has been changed to `ShowPartitions`.

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

### How was this patch tested?
Only update comments.

### Was this patch authored or co-authored using generative AI tooling?
No.

Closes apache#47562 from panbingkun/fix_astbuilder.

Lead-authored-by: panbingkun <[email protected]>
Co-authored-by: panbingkun <[email protected]>
Signed-off-by: yangjie01 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxGekk MaxGekk MaxGekk approved these changes

@imback82 imback82 imback82 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.
4 participants
@cloud-fan @SparkQA @MaxGekk @imback82