gaborkaszab commented on code in PR #6324:
URL: https://github.com/apache/iceberg/pull/6324#discussion_r1040783493
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -422,20 +424,29 @@ private Table newHmsTable(TableMetadata metadata) {
Preconditions.checkNotNull(metadata, "'metadata' parameter can't be null");
final long currentTimeMillis = System.currentTimeMillis();
- Table newTable =
- new Table(
- tableName,
- database,
- metadata.property(HiveCatalog.HMS_TABLE_OWNER,
System.getProperty("user.name")),
- (int) currentTimeMillis / 1000,
- (int) currentTimeMillis / 1000,
- Integer.MAX_VALUE,
- null,
- Collections.emptyList(),
- Maps.newHashMap(),
- null,
- null,
- TableType.EXTERNAL_TABLE.toString());
+ Table newTable;
+ try {
Review Comment:
Might be just personal preference but I'd find it more readable not to put
the while "new Table()" call into a try-catch block to cover
UGI.getCurrentUser(). I'd rather construct the user name before calling this
function. What do you think?
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -470,6 +481,22 @@ private void setHmsTableParameters(
// remove any props from HMS that are no longer present in Iceberg table
props
obsoleteProps.forEach(parameters::remove);
+ if (metadata.property(HiveCatalog.HMS_TABLE_OWNER, null) != null) {
Review Comment:
Agree @szehon-ho that this seems a different use-case for me. Might worth
covering it in a separate PR.
Additionally, this function is for setting the table parameters and with
this change it would also have a side effect of setting the tbl's owner that is
not really a table property. I would feel this is a bit off in my opinion.
And anyway, when this code was run for a "create table" scenario then
newHmsTable() would have already set the table owner so the below line would be
redundant.
But first of all as this is for the update scenario I'd say that this
deserve a separate PR because this one is for changing the defaults during
table creation.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java:
##########
@@ -567,8 +569,13 @@ Database convertToDatabase(Namespace namespace,
Map<String, String> meta) {
});
if (database.getOwnerName() == null) {
- database.setOwnerName(System.getProperty("user.name"));
- database.setOwnerType(PrincipalType.USER);
+ try {
+
database.setOwnerName(UserGroupInformation.getCurrentUser().getUserName());
+ database.setOwnerType(PrincipalType.USER);
+ } catch (IOException e) {
+ throw new RuntimeException(
Review Comment:
+1 for UncheckedIOException.
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java:
##########
@@ -470,6 +481,22 @@ private void setHmsTableParameters(
// remove any props from HMS that are no longer present in Iceberg table
props
obsoleteProps.forEach(parameters::remove);
+ if (metadata.property(HiveCatalog.HMS_TABLE_OWNER, null) != null) {
Review Comment:
I think this should be simpler:
`if (metadata.properties().containsKey(HiveCatalog.HMS_TABLE_OWNER)) {`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
