Skip to content

[SPARK-29695][SQL] ALTER TABLE (SerDe properties) should look up catalog/table like v2 commands #26374

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
huaxingao wants to merge 2 commits into from
Closed

[SPARK-29695][SQL] ALTER TABLE (SerDe properties) should look up catalog/table like v2 commands #26374

huaxingao wants to merge 2 commits into from

Conversation

@huaxingao
Copy link
Contributor

@huaxingao huaxingao commented Nov 3, 2019

What changes were proposed in this pull request?

Add AlterTableSerDePropertiesStatement and make ALTER TABLE ... SET SERDE/SERDEPROPERTIES 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
ALTER TABLE t SET SERDE 'org.apache.class' // 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 ALTER TABLE ... SET SERDE/SERDEPROPERTIES, 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?

Unit tests.

@SparkQA
Copy link

SparkQA commented Nov 3, 2019

Test build #113146 has finished for PR 26374 at commit b32975f.

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

@huaxingao
Copy link
Contributor Author

huaxingao commented Nov 3, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Nov 3, 2019

Test build #113180 has finished for PR 26374 at commit b32975f.

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

@huaxingao
Copy link
Contributor Author

huaxingao commented Nov 4, 2019

@viirya @cloud-fan @imback82
Could you please review? Thanks!

@cloud-fan
Copy link
Contributor

cloud-fan commented Nov 4, 2019

unfortunately it conflicts...

@huaxingao
Copy link
Contributor Author

huaxingao commented Nov 4, 2019

I actually just resolved the conflict a few mins ago before I pinged you. Not sure why it didn't refresh. Let me double check.

@huaxingao
Copy link
Contributor Author

huaxingao commented Nov 4, 2019

Should be good now. Thanks! @cloud-fan

imback82
imback82 approved these changes Nov 4, 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

dongjoon-hyun
dongjoon-hyun reviewed Nov 4, 2019
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

I left a few minor comments to polish the test part, @huaxingao . The other part looks good.

@SparkQA
Copy link

SparkQA commented Nov 4, 2019

Test build #113219 has finished for PR 26374 at commit 00eea57.

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

@huaxingao
Copy link
Contributor Author

huaxingao commented Nov 4, 2019

@dongjoon-hyun Thanks for your comments. I will update the code.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Nov 4, 2019

Thanks!

dongjoon-hyun
dongjoon-hyun approved these changes Nov 5, 2019
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM (Pending Jenkins).
Thank you, @huaxingao .

@SparkQA
Copy link

SparkQA commented Nov 5, 2019

Test build #113231 has finished for PR 26374 at commit a38b170.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Nov 5, 2019 •
edited
Loading

Merged to master.
Thank you, @huaxingao , @cloud-fan , @viirya , @imback82

@huaxingao
Copy link
Contributor Author

huaxingao commented Nov 5, 2019

Thanks a lot! @dongjoon-hyun @cloud-fan @viirya @imback82

@huaxingao huaxingao deleted the spark_29695 branch November 5, 2019 06:03
@SparkQA
Copy link

SparkQA commented Nov 5, 2019

Test build #113233 has finished for PR 26374 at commit 7c2078f.

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

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

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

@dongjoon-hyun dongjoon-hyun dongjoon-hyun 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

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