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.

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

[SPARK-21112] [SQL] ALTER TABLE SET TBLPROPERTIES should not overwrite COMMENT #18318

Closed
wants to merge 1 commit into from

Conversation

gatorsmile
Copy link
Member

@gatorsmile gatorsmile commented Jun 15, 2017

What changes were proposed in this pull request?

ALTER TABLE SET TBLPROPERTIES should not overwrite COMMENT even if the input property does not have the property of COMMENT. This PR is to fix the issue.

How was this patch tested?

Covered by the existing tests.

@gatorsmile
Copy link
Member Author

cc @wzhfy @cloud-fan

@@ -235,7 +235,7 @@ case class AlterTableSetPropertiesCommand(
// direct property.
val newTable = table.copy(
properties = table.properties ++ properties,
comment = properties.get("comment"))
comment = properties.get("comment").orElse(table.comment))
Copy link
Contributor

Choose a reason for hiding this comment

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

What about UNSET TBLPROPERTIES command?

Copy link
Member

Choose a reason for hiding this comment

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

This looks fine. However, when setting properties for Hive table, a None comment will be ignored and won't override the comment.

We don't do similar thing in InMemoryCatalog and just override it with the None comment.

Not sure if we should fix it here or in InMemoryCatalog.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you show an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

alter table src set tblproperties ('foo' = 'bar', 'comment' = 'table_comment');
alter table src unset tblproperties ('foo');
we will lost comment in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@SparkQA
Copy link

SparkQA commented Jun 16, 2017

Test build #78123 has finished for PR 18318 at commit c697f9a.

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

@cloud-fan
Copy link
Contributor

LGTM, merging to master!

@cloud-fan
Copy link
Contributor

is it also a problem for other branches?

@asfgit asfgit closed this in 5d35d5c Jun 16, 2017
@gatorsmile
Copy link
Member Author

Only the master branch has such an issue. Thanks!

dataknocker pushed a commit to dataknocker/spark that referenced this pull request Jun 16, 2017
… COMMENT

### What changes were proposed in this pull request?
`ALTER TABLE SET TBLPROPERTIES` should not overwrite `COMMENT` even if the input property does not have the property of `COMMENT`. This PR is to fix the issue.

### How was this patch tested?
Covered by the existing tests.

Author: Xiao Li <gatorsmile@gmail.com>

Closes apache#18318 from gatorsmile/fixTableComment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants