laskoviymishka commented on code in PR #16620:
URL: https://github.com/apache/iceberg/pull/16620#discussion_r3528841926
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -172,6 +172,21 @@ protected HiveCatalog catalog() {
return catalog;
}
+ @Test
+ public void newTableSetsCurrentHmsLastAccessTime() throws TException {
+ TableIdentifier tableIdent = TableIdentifier.of(DB_NAME,
"create_time_tbl");
+
+ int beforeSeconds = (int) (System.currentTimeMillis() / 1000);
+ catalog.createTable(tableIdent, getTestSchema());
+ int afterSeconds = (int) (System.currentTimeMillis() / 1000);
+
+ org.apache.hadoop.hive.metastore.api.Table hmsTable =
+ HIVE_METASTORE_EXTENSION.metastoreClient().getTable(DB_NAME,
"create_time_tbl");
Review Comment:
the `"create_time_tbl"` literal is repeated here and in the
`TableIdentifier.of` above — a typo in one would create under one name and
query another, which surfaces as an HMS-lookup error rather than a clear
assertion failure. The view test already dodges this with `identifier.name()`;
I'd do the same here:
```java
HIVE_METASTORE_EXTENSION.metastoreClient().getTable(DB_NAME,
tableIdent.name());
```
##########
hive-metastore/src/main/java/org/apache/iceberg/hive/HiveOperationsBase.java:
##########
@@ -206,15 +206,17 @@ static void cleanupMetadataAndUnlock(
default Table newHmsTable(String hmsTableOwner) {
Preconditions.checkNotNull(hmsTableOwner, "'hmsOwner' parameter can't be
null");
- final long currentTimeMillis = System.currentTimeMillis();
+ // createTime and lastAccessTime are HMS Thrift i32 fields (epoch
seconds), so the value must be
+ // narrowed to int here; the width is dictated by the metastore protocol
and cannot be widened.
Review Comment:
agreed the narrowing is protocol-dictated, but "cannot be widened" reads as
a permanent invariant when it's really "as of the HMS Thrift API we build
against here" — Hive 4.x has already grown `CreateTableRequest`, so I'd soften
it so a future maintainer on a newer client doesn't take it as gospel.
This same two-line comment is also duplicated verbatim in
`HiveViewOperations.newHMSView`. Since `currentTimeSeconds` is fairly
self-documenting, I'd trim both to a single line, something like `// epoch
seconds; HMS Thrift stores these as i32`.
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveViewCatalog.java:
##########
@@ -100,6 +100,30 @@ protected boolean requiresNamespaceCreate() {
return true;
}
+ @Test
+ public void newViewSetsCurrentHmsLastAccessTime() throws Exception {
Review Comment:
tiny consistency thing — this is the only method in the file that declares
`throws Exception`; the rest of the class (and the sibling table test) uses
`throws TException`, which is all this actually throws. I'd narrow it to match.
--
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]
