Skip to content

[SPARK-38939][SQL][FOLLOWUP] Replace named parameter with comment in ReplaceColumns - #36487

Closed
dcoliversun wants to merge 1 commit into
apache:masterfrom
dcoliversun:SPARK-38939
Closed

[SPARK-38939][SQL][FOLLOWUP] Replace named parameter with comment in ReplaceColumns#36487
dcoliversun wants to merge 1 commit into
apache:masterfrom
dcoliversun:SPARK-38939

Conversation

@dcoliversun

@dcoliversundcoliversun commented May 9, 2022

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This PR aims to replace named parameter with comment in ReplaceColumns.

Why are the changes needed?

#36252 changed signature of deleteColumn#TableChange.java, but this PR breaks sbt compilation in k8s integration test.

> build/sbt -Pkubernetes -Pkubernetes-integration-tests -Dtest.exclude.tags=r -Dspark.kubernetes.test.imageRepo=kubespark "kubernetes-integration-tests/test"
[error] /Users/IdeaProjects/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala:147:45: not found: value ifExists
[error] TableChange.deleteColumn(Array(name), ifExists = false)
[error] ^
[error] /Users/IdeaProjects/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala:159:19: value ++ is not a member of Array[Nothing]
[error] deleteChanges ++ addChanges
[error] ^
[error] two errors found
[error] (catalyst / Compile / compileIncremental) Compilation failed

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass the GA and k8s integration test.

@dcoliversun

Copy link
Copy Markdown
ContributorAuthor

cc @cloud-fan@jackierwzhang

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry I'm a bit confused. named parameter has been widely used in the Spark codebase, what's wrong with it?

@dcoliversundcoliversunMay 9, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, named parameter is right. But there is a compilation error when k8s integration test. Please see PR description for more information. And you can exec this command to check it again

build/sbt -Pkubernetes -Pkubernetes-integration-tests -Dtest.exclude.tags=r "kubernetes-integration-tests/test"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you know why the issue only happens in this place?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Because deleteColumn#TableChange is Java file. I replace named parameter with comment, because it's used in Apache Spark.

blockHandler.applicationRemoved(appId, true/* cleanupLocalDirs */)

@dcoliversundcoliversun changed the title [SPARK-38939][SQL][FOLLOWUP] Remove undefined variable 'ifExists'[SPARK-38939][SQL][FOLLOWUP] Replace named parameter with comment in ReplaceColumnsMay 9, 2022
@AmplabJenkins

Copy link
Copy Markdown

Can one of the admins verify this patch?

huaxingao pushed a commit that referenced this pull request May 9, 2022
…ReplaceColumns
### What changes were proposed in this pull request?
This PR aims to replace named parameter with comment in `ReplaceColumns`.
### Why are the changes needed?
#36252 changed signature of deleteColumn#**TableChange.java**, but this PR breaks sbt compilation in k8s integration test.
```shell
> build/sbt -Pkubernetes -Pkubernetes-integration-tests -Dtest.exclude.tags=r -Dspark.kubernetes.test.imageRepo=kubespark "kubernetes-integration-tests/test"
[error] /Users/IdeaProjects/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala:147:45: not found: value ifExists
[error] TableChange.deleteColumn(Array(name), ifExists = false)
[error] ^
[error] /Users/IdeaProjects/spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/v2AlterTableCommands.scala:159:19: value ++ is not a member of Array[Nothing]
[error] deleteChanges ++ addChanges
[error] ^
[error] two errors found
[error] (catalyst / Compile / compileIncremental) Compilation failed
```
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass the GA and k8s integration test.
Closes#36487 from dcoliversun/SPARK-38939.
Authored-by: Qian.Sun <qian.sun2020@gmail.com>
Signed-off-by: huaxingao <huaxin_gao@apple.com>
(cherry picked from commit 16b5124)
Signed-off-by: huaxingao <huaxin_gao@apple.com>
@huaxingao

Copy link
Copy Markdown
Contributor

Thanks! Merged to master and 3.3.

@dcoliversun

Copy link
Copy Markdown
ContributorAuthor

Thank you @cloud-fan@jackierwzhang@huaxingao

@dcoliversun
dcoliversun deleted the SPARK-38939 branch May 10, 2022 00:48
Sign up for freeto 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.

5 participants

@dcoliversun@AmplabJenkins@huaxingao@cloud-fan@jackierwzhang