nastra commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r920166617
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -65,6 +66,26 @@ public Table loadTable(TableIdentifier identifier) {
return result;
}
+ @Override
+ public Table registerTable(TableIdentifier identifier, String
metadataFileLocation) {
+ Preconditions.checkArgument(
+ (identifier != null) && isValidIdentifier(identifier), "Invalid
identifier: %s", identifier);
Review Comment:
nit: you can probably omit the parentheses around the null check here
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -65,6 +66,26 @@ public Table loadTable(TableIdentifier identifier) {
return result;
}
+ @Override
+ public Table registerTable(TableIdentifier identifier, String
metadataFileLocation) {
+ Preconditions.checkArgument(
+ (identifier != null) && isValidIdentifier(identifier), "Invalid
identifier: %s", identifier);
+ Preconditions.checkArgument(metadataFileLocation != null &&
!metadataFileLocation.isEmpty(),
+ "Cannot register an empty metadata file location as a table");
+
+ // Throw an exception if this table already exists in the catalog.
Review Comment:
I think it makes sense to adjust the impl here to what Ryan suggested in
https://github.com/apache/iceberg/pull/4099/files#r805404106
##########
core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java:
##########
@@ -639,4 +642,29 @@ public void testConversions() {
Assert.assertEquals(ns, JdbcUtil.stringToNamespace(nsString));
}
+ @Test
+ public void testRegisterTable() {
Review Comment:
is this something that could be moved to `CatalogTests` so that it can be
tested with different catalog implementations?
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -385,6 +388,89 @@ public void testDropTable() throws IOException {
verifyCommitMetadata();
}
+ private void testRegister(TableIdentifier identifier, String
metadataVersionFiles) {
+ Assertions.assertThat(catalog.registerTable(identifier, "file:" +
metadataVersionFiles)).isNotNull();
+ Table newTable = catalog.loadTable(identifier);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations)
ops).currentMetadataLocation();
+ Assertions.assertThat("file:" +
metadataVersionFiles).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.dropTable(identifier, false)).isTrue();
+ }
+
+ @Test
+ public void testRegisterTableWithGivenBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ ImmutableTableReference tableReference =
+
ImmutableTableReference.builder().reference("main").name(TABLE_NAME).build();
+ TableIdentifier identifier = TableIdentifier.of(DB_NAME,
tableReference.toString());
+ testRegister(identifier, metadataVersionFiles.get(0));
+ }
+
+ @Test
+ public void testRegisterTableNegativeScenarios() throws
NessieConflictException, NessieNotFoundException {
Review Comment:
this is quite a long method. Maybe just create a new test class specifically
for testing Table registration and then separate things to be tested into their
own test methods? That way the code would get cleaner and easier to read
--
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]
