szehon-ho commented on code in PR #6591:
URL: https://github.com/apache/iceberg/pull/6591#discussion_r1082932748
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##########
@@ -154,6 +154,12 @@ protected void disableRefresh() {
this.shouldRefresh = false;
}
+ protected String writeNewMetadataIfRequired(boolean isCreateTable,
TableMetadata metadata) {
Review Comment:
Should we use this in HiveTableOperations as well?
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java:
##########
@@ -154,6 +154,12 @@ protected void disableRefresh() {
this.shouldRefresh = false;
}
+ protected String writeNewMetadataIfRequired(boolean isCreateTable,
TableMetadata metadata) {
Review Comment:
I think we can remove 'is' in 'isCreateTable' following guideline for
boolean args. Ref: https://iceberg.apache.org/contribute/#java-style-guidelines
In this case, what do you think of calling it 'newTable'? (createTable will
be a bit like dictating whether we want to create a table or not, whereas here
we are describing whether the table is new)
##########
core/src/main/java/org/apache/iceberg/jdbc/JdbcTableOperations.java:
##########
@@ -102,7 +102,7 @@ public void doRefresh() {
@Override
public void doCommit(TableMetadata base, TableMetadata metadata) {
- String newMetadataLocation = writeNewMetadata(metadata, currentVersion() +
1);
+ String newMetadataLocation = writeNewMetadataIfRequired(base == null,
metadata);
Review Comment:
Also to me, its clearer to add an extra line :
```
boolean newTable = base == null;
String newMetadataLocation = writeNewMetadataIfRequired(newTable, metadata);
```
(if we keep the name I suggested above).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
