laskoviymishka commented on code in PR #16620:
URL: https://github.com/apache/iceberg/pull/16620#discussion_r3480732118
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java:
##########
@@ -172,6 +172,19 @@ 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");
+ assertThat(hmsTable.getLastAccessTime()).isBetween(beforeSeconds,
afterSeconds);
Review Comment:
The fix sets both `createTime` and `lastAccessTime`, but this test only
asserts `lastAccessTime` — so the `createTime` half of the change isn't
actually exercised here. If that arg got reverted or swapped back to the old
`(int) currentTimeMillis / 1000`, this test would still pass.
The question is whether the embedded HMS in these tests persists the
client-supplied `createTime` or overwrites it server-side the way a real
`CreateTableHandler` does. If it persists it, I'd add
`assertThat(hmsTable.getCreateTime()).isBetween(beforeSeconds, afterSeconds)`
right here so both fields are covered. If it overwrites it, a one-line comment
saying so would make the asymmetry intentional and obvious. Either way, the
same applies to the view test in `TestHiveViewCatalog`.
--
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]
