-
Notifications
You must be signed in to change notification settings - Fork 28.6k
[SPARK-29405][SQL] Alter table / Insert statements should not change a table's ownership #26068
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
Conversation
…a table's ownership
cc @wangyum |
retest this please |
Test build #111952 has finished for PR 26068 at commit
|
Test build #111964 has finished for PR 26068 at commit
|
@@ -573,8 +574,9 @@ private[hive] class HiveClientImpl( | |||
// If users explicitly alter these Hive-specific properties through ALTER TABLE DDL, we respect | |||
// these user-specified values. | |||
verifyColumnDataType(table.dataSchema) | |||
val owner = Option(table.owner).filter(_.nonEmpty).getOrElse(userName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move this change to HiveClientImpl.scala#L1043?
Option(table.owner).filter(_.nonEmpty).orElse(userName).foreach(hiveTable.setOwner)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It sounds like a bigger deal than it is. Indeed, I am going to add alter table owner
syntax, which may make this kind of change back and forth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, yea I think @wangyum's suggestion is more correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @HyukjinKwon Fixed by #26160.
also cc @cloud-fan |
how is INSERT TABLE fixed in this PR? |
spark/sql/hive/src/main/scala/org/apache/spark/sql/hive/execution/InsertIntoHiveTable.scala Line 112 in a45739d
would eventually call alterTable here
|
can we add a test in |
Test build #112206 has finished for PR 26068 at commit
|
sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala
Outdated
Show resolved
Hide resolved
To clarify, this PR fixed the case of |
Test build #112215 has finished for PR 26068 at commit
|
Test build #112214 has finished for PR 26068 at commit
|
thanks, merging to master! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM too, though.
What changes were proposed in this pull request?
In this change, we give preference to the original table's owner if it is not empty.
Why are the changes needed?
When executing 'insert into/overwrite ...' DML, or 'alter table set tblproperties ...' DDL, spark would change the ownership of the table the one who runs the spark application.
Does this PR introduce any user-facing change?
NO
How was this patch tested?
Compare with the behavior of Apache Hive