You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/06/14 20:39:19 UTC
[GitHub] [iceberg] Mehul2500 opened a new pull request, #5037: [WIP] Implementation of BaseMetastoreCatalog.registerTable()
Mehul2500 opened a new pull request, #5037:
URL: https://github.com/apache/iceberg/pull/5037
For migrating tables between different catalogs, need is there for a registerTable() function on destination catalog side.
Thus, planned to implement the registerTable() function in BaseMetastoreCatalog.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r902406762
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -196,7 +194,16 @@ public List<TableIdentifier> listTables(Namespace namespace) {
@Override
public boolean dropTable(TableIdentifier identifier, boolean purge) {
- return client.dropTable(identifier, purge);
+ TableReference tableReference = parseTableReference(identifier);
Review Comment:
Please mention in PR description that these changes are from #5033
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r901487814
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +361,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, List<String> metadataVersionFiles) {
+ Assertions.assertThat(catalog.dropTable(identifier, false)).isFalse();
+ Assertions.assertThat(catalog.registerTable(
+ identifier,
+ "file:" + metadataVersionFiles.get(0))).isNotNull();
+ Table newTable = catalog.loadTable(identifier);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(identifier)).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);
+ }
+
+ @Test
+ public void testRegisterTableNegativeScenarios() throws NessieConflictException, NessieNotFoundException {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ // Case 1: Branch does not exist
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@default"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Nessie ref 'default' does not exist");
+ // Case 2: Table Already Exists
+ Assertions.assertThatThrownBy(() -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(AlreadyExistsException.class)
+ .hasMessage(
+ "Table already exists: db.tbl");
+ // Case 3: Registering using a tag
+ api.createReference().sourceRefName(BRANCH).reference(Tag.of("tag_1", catalog.currentHash())).create();
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@tag_1"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("You can only mutate tables when using a branch without a hash or timestamp.");
+ // Case 4: non-null metadata path with null metadata location
+ Assertions.assertThat(catalog.dropTable(TABLE_IDENTIFIER, false)).isTrue();
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0) + "invalidName"))
+ .isInstanceOf(NotFoundException.class);
+ }
+
+ @Test
+ public void testRegisterTableWithDefaultBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ Assertions.assertThat(catalog.dropTable(TABLE_IDENTIFIER)).isTrue();
+ Assertions.assertThat(catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0))).isNotNull();
Review Comment:
We cannot do so as the line wherein we assert for dropTable in this particular test case is different to the assertion we implemented in testRegister(), because of the dropTable backend bugs.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r914507239
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,81 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {
Review Comment:
I got my AWS account setup, and have amended the test cases in Glue and DynamoDb Catalog, @pvary please have a look over the test cases.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r901517898
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +361,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, List<String> metadataVersionFiles) {
+ Assertions.assertThat(catalog.dropTable(identifier, false)).isFalse();
+ Assertions.assertThat(catalog.registerTable(
+ identifier,
+ "file:" + metadataVersionFiles.get(0))).isNotNull();
+ Table newTable = catalog.loadTable(identifier);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(identifier)).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);
Review Comment:
ok
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r920345031
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,81 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {
Review Comment:
> I do not like the duplicated test code, and I think it would be good to have a general Catalog API test for checking the functionality of the new Catalog implementations. Something like TestCatalog but for all/most of the catalog API methods. We can do it in a different PR, but it would be good to have them. The test for registerTable() is a good candidate for these common tests.
We do have `CatalogTests`, which is what `TestRESTCatalog` is built off of. It’s nice in that it has some methods for subclasses to declare if the catalog being tested supports certain features, so the tests can exit early if they don’t support an optional feature.
For example if the catalog in question doesn’t support namespace properties.
Might be good to see if that can be made to fit this need or can otherwise get some ideas from it. I believe Nessie also implements these tests.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r899776723
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -65,6 +66,22 @@ public Table loadTable(TableIdentifier identifier) {
return result;
}
+ @Override
+ public org.apache.iceberg.Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ Preconditions.checkArgument(isValidIdentifier(identifier), "Invalid identifier: %s", identifier);
+
+ // Throw an exception if this table already exists in the catalog.
+ if (tableExists(identifier)) {
+ throw new org.apache.iceberg.exceptions.AlreadyExistsException("Table already exists: %s", identifier);
+ }
+
+ TableOperations ops = newTableOps(identifier);
+ InputFile metadataFile = ops.io().newInputFile(metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(ops.io(), metadataFile);
+ ops.commit(null, metadata);
Review Comment:
It shows up, 'Cannot call commit on temporary table operations', thus continuing with the previous code statement,i.e., line 81.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r910177760
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -65,6 +66,26 @@ public Table loadTable(TableIdentifier identifier) {
return result;
}
+ @Override
+ public org.apache.iceberg.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.
+ if (tableExists(identifier)) {
+ throw new org.apache.iceberg.exceptions.AlreadyExistsException("Table already exists: %s", identifier);
Review Comment:
Why does this need to be fully qualified?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r902476755
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -65,6 +66,26 @@ public Table loadTable(TableIdentifier identifier) {
return result;
}
+ @Override
+ public org.apache.iceberg.Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ Preconditions.checkArgument(
+ isValidIdentifier(identifier) && identifier != null, "Invalid identifier: %s", identifier);
Review Comment:
it would be more natural to do the null check first
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r905927196
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,81 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {
Review Comment:
If I understand correctly, the PR moves the `HiveCatalog.registerTable` to the `BaseMetastoreCatalog` so it will be available for every `Catalog` which is inheriting from `BaseMetastoreCatalog`. So we should have a test for testing all of the catalogs that implement the `registerTable` that the method is working as expected.
I understand that there are some Nessie specific test cases, but I think the basic functionality should be available and tested for all of the catalogs.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r902491274
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -196,7 +194,16 @@ public List<TableIdentifier> listTables(Namespace namespace) {
@Override
public boolean dropTable(TableIdentifier identifier, boolean purge) {
- return client.dropTable(identifier, purge);
+ TableReference tableReference = parseTableReference(identifier);
Review Comment:
@pvary: please help merging #5033, So that we can rebase this.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r905870194
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,81 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {
Review Comment:
These test cases are specific to Nessie.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r921579263
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,81 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {
Review Comment:
A good suggestion of using `CatalogTests`, but the duplicated test code referred to under these comments above shall not be resolved using `CatalogTests`.
Reason:
1. The duplicated test code is with two of the test cases, i.e., testRegisterTable() and testRegisterExistingTable() in three of the catalog test files.
- Jdbc Catalog
- Ecs Catalog
- Hadoop Catalog
Out of these only Jdbc catalog is one which can use `CatalogTests`, others do not use `CatalogTests`.
Thus, it won't be helpful to consider `CatalogTests` under this comment.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
dimas-b commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r920180053
##########
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) {
Review Comment:
This is a utility method, but it is named like a test method... Would you mind renaming to something like `validateRegister`?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r902481216
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -152,7 +152,8 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
}
}
- String newMetadataLocation = writeNewMetadata(metadata, currentVersion() + 1);
+ String newMetadataLocation = base == null && metadata.metadataFileLocation() != null ?
Review Comment:
nit: maybe use parenthesis around the condition, so it is 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r913415909
##########
aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java:
##########
@@ -295,6 +299,30 @@ public void testDropNamespace() {
Assert.assertFalse("namespace must not exist", response.hasItem());
}
+ @Ignore
+ public void testRegisterTable() {
+ TableIdentifier identifier = TableIdentifier.of("a", "t1");
Review Comment:
probably gets a namespace doesn't exist error. Need to create a namespace first? Also good to use `genRandomName()` as in other testcases.
##########
aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java:
##########
@@ -295,6 +299,30 @@ public void testDropNamespace() {
Assert.assertFalse("namespace must not exist", response.hasItem());
}
+ @Ignore
+ public void testRegisterTable() {
+ TableIdentifier identifier = TableIdentifier.of("a", "t1");
+ catalog.createTable(identifier, SCHEMA);
+ Table registeringTable = catalog.loadTable(identifier);
+ catalog.dropTable(identifier, false);
+ TableOperations ops = ((HasTableOperations) registeringTable).operations();
+ String metadataLocation = ((DynamoDbTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat(catalog.registerTable(identifier, metadataLocation)).isNotNull();
+ Table newTable = catalog.loadTable(identifier);
+ Assertions.assertThat(newTable).isNotNull();
+ }
+
+ @Ignore
+ public void testRegisterExistingTable() {
+ catalog.createTable(TableIdentifier.of("a", "t1"), SCHEMA);
Review Comment:
make a table identifier once and use in all 3 places.
and same comments as above.
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -433,4 +435,28 @@ public void testTablePropsDefinedAtCatalogLevel() {
"table-key5",
table.properties().get("key5"));
}
+
+ @Ignore
+ public void testRegisterTable() {
+ String namespace = createNamespace();
+ String tableName = getRandomName();
+ createTable(namespace, tableName);
+ Table table = glueCatalog.loadTable(TableIdentifier.of(namespace, tableName));
+ String metadataLocation = ((BaseTable) table).operations().current().metadataFileLocation();
+ glueCatalog.dropTable(TableIdentifier.of(namespace, tableName), false);
+ glueCatalog.registerTable(TableIdentifier.of(namespace, tableName), metadataLocation);
Review Comment:
need to validate the results and test load table?
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -589,4 +593,31 @@ public void testTablePropsDefinedAtCatalogLevel() throws IOException {
"table-key5",
table.properties().get("key5"));
}
+
+ @Test
+ public void testRegisterTable() throws IOException {
+ TableIdentifier identifier = TableIdentifier.of("a", "t1");
+ TableIdentifier identifier2 = TableIdentifier.of("a", "t2");
+ HadoopCatalog catalog = hadoopCatalog();
+ catalog.createTable(identifier, SCHEMA);
+ Table registeringTable = catalog.loadTable(identifier);
+ TableOperations ops = ((HasTableOperations) registeringTable).operations();
+ String metadataLocation = ((HadoopTableOperations) ops).current().metadataFileLocation();
+ Assertions.assertThat(catalog.registerTable(identifier2, metadataLocation)).isNotNull();
+ Table newTable = catalog.loadTable(identifier2);
Review Comment:
nit: no need for the variable if it is used only once. Can be
`Assertions.assertThat(catalog.loadTable(identifier2)).isNotNull();`
##########
aws/src/integration/java/org/apache/iceberg/aws/glue/TestGlueCatalogTable.java:
##########
@@ -433,4 +435,28 @@ public void testTablePropsDefinedAtCatalogLevel() {
"table-key5",
table.properties().get("key5"));
}
+
+ @Ignore
+ public void testRegisterTable() {
+ String namespace = createNamespace();
+ String tableName = getRandomName();
+ createTable(namespace, tableName);
+ Table table = glueCatalog.loadTable(TableIdentifier.of(namespace, tableName));
+ String metadataLocation = ((BaseTable) table).operations().current().metadataFileLocation();
+ glueCatalog.dropTable(TableIdentifier.of(namespace, tableName), false);
+ glueCatalog.registerTable(TableIdentifier.of(namespace, tableName), metadataLocation);
+ }
+
+ @Ignore
+ public void testRegisterTableAlreadyExists() {
+ String namespace = createNamespace();
+ String tableName = getRandomName();
+ createTable(namespace, tableName);
+ Table table = glueCatalog.loadTable(TableIdentifier.of(namespace, tableName));
+ String metadataLocation = ((BaseTable) table).operations().current().metadataFileLocation();
+ AssertHelpers.assertThrows("Should fail to register to an existing Glue table",
+ AlreadyExistsException.class,
+ "already exists in Glue",
Review Comment:
you many not get this error message `"already exists in Glue"`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r921587176
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -152,7 +152,8 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
}
}
- String newMetadataLocation = writeNewMetadata(metadata, currentVersion() + 1);
+ String newMetadataLocation = (base == null) && (metadata.metadataFileLocation() != null) ?
+ metadata.metadataFileLocation() : writeNewMetadata(metadata, currentVersion() + 1);
Review Comment:
I have considered the changes similar to the `HiveTableOperations` file
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r901437936
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -65,6 +66,24 @@ public Table loadTable(TableIdentifier identifier) {
return result;
}
+ @Override
+ public org.apache.iceberg.Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ Preconditions.checkArgument(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.
+ if (tableExists(identifier)) {
+ throw new org.apache.iceberg.exceptions.AlreadyExistsException("Table already exists: %s", identifier);
+ }
+
+ TableOperations ops = newTableOps(identifier);
+ InputFile metadataFile = ops.io().newInputFile(metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(ops.io(), metadataFile);
+ ops.commit(null, metadata);
+
+ return new BaseTable(ops, identifier.toString());
+ }
Review Comment:
please have a new line here.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r901447210
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -65,6 +66,24 @@ public Table loadTable(TableIdentifier identifier) {
return result;
}
+ @Override
+ public org.apache.iceberg.Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ Preconditions.checkArgument(isValidIdentifier(identifier), "Invalid identifier: %s", identifier);
Review Comment:
Need to have a null check for the identifier?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r921586535
##########
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) {
Review Comment:
updated...thank you
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r916513979
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -385,6 +388,83 @@ 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 {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ // Case 1: Branch does not exist
+ ImmutableTableReference tableReference =
+ ImmutableTableReference.builder().reference("default").name(TABLE_NAME).build();
+ TableIdentifier identifier = TableIdentifier.of(DB_NAME, tableReference.toString());
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ identifier, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Nessie ref 'default' does not exist");
+ // Case 2: Table Already Exists
+ Assertions.assertThatThrownBy(() -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(AlreadyExistsException.class)
+ .hasMessage("Table already exists: db.tbl");
+ // Case 3: Registering using a tag
+ api.createReference().sourceRefName(BRANCH).reference(Tag.of("tag_1", catalog.currentHash())).create();
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@tag_1"),
Review Comment:
nit: can use `TableReference` here for tag also.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r912845641
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,81 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {
Review Comment:
I have put in test cases for Glue and DynamoDb as "Ignore" because I currently do not have an AWS account for testing them.
Also, I feel we could address the Catalog API test in a different PR.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r921103235
##########
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:
A similar comment was posted out by @ajantha-bhat, please refer https://github.com/apache/iceberg/pull/5037#discussion_r898211187
TLDR it gives an error
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r901447806
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -65,6 +66,22 @@ public Table loadTable(TableIdentifier identifier) {
return result;
}
+ @Override
+ public org.apache.iceberg.Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ Preconditions.checkArgument(isValidIdentifier(identifier), "Invalid identifier: %s", identifier);
+
+ // Throw an exception if this table already exists in the catalog.
+ if (tableExists(identifier)) {
+ throw new org.apache.iceberg.exceptions.AlreadyExistsException("Table already exists: %s", identifier);
+ }
+
+ TableOperations ops = newTableOps(identifier);
+ InputFile metadataFile = ops.io().newInputFile(metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(ops.io(), metadataFile);
+ ops.commit(null, metadata);
Review Comment:
ok. Intersting.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r912845641
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,81 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {
Review Comment:
I have put in test cases for Glue and DynamoDb as "Ignore" because I currently do not have an AWS account for testing them. I working on it.
Also, I would address the general Catalog API test in a follow-up PR.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r906300178
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,81 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {
Review Comment:
I will add the said test-cases and would ping you then, thanks.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: [WIP] Implementation of BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r898206634
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -65,6 +66,22 @@ public Table loadTable(TableIdentifier identifier) {
return result;
}
+ @Override
+ public org.apache.iceberg.Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ Preconditions.checkArgument(isValidIdentifier(identifier), "Invalid identifier: %s", identifier);
+
Review Comment:
I know we just moved this `HiveCatalog`.
But we can add the below validation
```
Preconditions.checkArgument(metadataFileLocation != null && !metadataFileLocation.isEmpty(),
"Cannot register an empty metadata file location as a table");
```
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ @Test
+ public void testRegisterTableWithGivenBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"), false);
+ catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"),
+ "file:" + metadataVersionFiles.get(0));
+ Table newTable = catalog.loadTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"));
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"))).isTrue();
+ }
+
+ @Test
+ public void testRegisterTableBranchNotPresent() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@default"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Nessie ref 'default' does not exist");
+ }
+
+ @Test
+ public void testRegisterTableWithDefaultBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TABLE_IDENTIFIER);
+ catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0));
+
+ Table newTable = catalog.loadTable(TABLE_IDENTIFIER);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
+ }
+
+ @Test
+ public void testRegisterTableTableAlreadyExists() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+
+ Assertions.assertThatThrownBy(() -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(AlreadyExistsException.class)
+ .hasMessage(
+ "Table already exists: db.tbl");
+ }
+
+ @Test
+ public void testRegisterTableTagName() throws NessieConflictException, NessieNotFoundException {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ api.createReference().sourceRefName(BRANCH).reference(Tag.of("tag_1", catalog.currentHash())).create();
+
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@tag_1"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("You can only mutate tables when using a branch without a hash or timestamp.");
+ }
+
+ @Ignore
+ public void testRegisterTableMoreThanOneBranch() throws NessieConflictException, NessieNotFoundException {
+ // Dependency on PR #5033 for the backend bug fixes in drop table.
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"), false);
+ catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"),
+ "file:" + metadataVersionFiles.get(0));
+ Table newTable = catalog.loadTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"));
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"))).isTrue();
+
+ api.createReference().sourceRefName(BRANCH).reference(Tag.of("branch1", catalog.currentHash())).create();
+
+ List<String> metadataVersionFiles1 = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles1.size());
+ catalog.dropTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@branch1`"));
+ catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@branch1`"),
+ "file:" + metadataVersionFiles1.get(0));
+ Table newTable1 = catalog.loadTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@branch1`"));
+ Assertions.assertThat(newTable1).isNotNull();
+ TableOperations ops1 = ((HasTableOperations) newTable1).operations();
+ String metadataLocation1 = ((NessieTableOperations) ops1).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles1.get(0)).isEqualTo(metadataLocation1);
+ Assertions.assertThat(catalog.tableExists(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@branch1`"))).isTrue();
+ }
+
private String getTableBasePath(String tableName) {
String databasePath = temp.toString() + "/" + DB_NAME;
return Paths.get(databasePath, tableName).toAbsolutePath().toString();
Review Comment:
We can add `registerTable` test cases of `GlueCatalog` also in this PR or follow-up PR.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ @Test
+ public void testRegisterTableWithGivenBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"), false);
+ catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"),
+ "file:" + metadataVersionFiles.get(0));
+ Table newTable = catalog.loadTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"));
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"))).isTrue();
+ }
+
+ @Test
+ public void testRegisterTableBranchNotPresent() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@default"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Nessie ref 'default' does not exist");
+ }
+
+ @Test
+ public void testRegisterTableWithDefaultBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TABLE_IDENTIFIER);
+ catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0));
+
+ Table newTable = catalog.loadTable(TABLE_IDENTIFIER);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
+ }
+
+ @Test
+ public void testRegisterTableTableAlreadyExists() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+
+ Assertions.assertThatThrownBy(() -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(AlreadyExistsException.class)
+ .hasMessage(
+ "Table already exists: db.tbl");
+ }
+
+ @Test
+ public void testRegisterTableTagName() throws NessieConflictException, NessieNotFoundException {
Review Comment:
you can combine all 3 negative scenario test case (that is checking Assertions.assertThatThrownBy) into one.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ @Test
+ public void testRegisterTableWithGivenBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"), false);
+ catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"),
+ "file:" + metadataVersionFiles.get(0));
+ Table newTable = catalog.loadTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"));
+ Assertions.assertThat(newTable).isNotNull();
Review Comment:
can you extract line 372 to 376 in a common function and reuse in other the testcases?
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -65,6 +66,22 @@ public Table loadTable(TableIdentifier identifier) {
return result;
}
+ @Override
+ public org.apache.iceberg.Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
+ Preconditions.checkArgument(isValidIdentifier(identifier), "Invalid identifier: %s", identifier);
+
+ // Throw an exception if this table already exists in the catalog.
+ if (tableExists(identifier)) {
+ throw new org.apache.iceberg.exceptions.AlreadyExistsException("Table already exists: %s", identifier);
+ }
+
+ TableOperations ops = newTableOps(identifier);
+ InputFile metadataFile = ops.io().newInputFile(metadataFileLocation);
+ TableMetadata metadata = TableMetadataParser.read(ops.io(), metadataFile);
+ ops.commit(null, metadata);
Review Comment:
Instead of line 81, can we have it like this as @rdblue suggested in https://github.com/apache/iceberg/pull/4099/files#r805404106
```
try {
// use temporary ops to pick up the table metadata settings
ops.temp(metadata).commit(null, metadata);
} catch (CommitFailedException ignored) {
throw new AlreadyExistsException("Table was created concurrently: %s", identifier);
}
```
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ @Test
+ public void testRegisterTableWithGivenBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"), false);
Review Comment:
nit: code looks cleaner if we use `TableReference` instead of \`tableName@branch\` syntax.
You can refer the example here
https://github.com/apache/iceberg/pull/5033/files#diff-a92127e5d74e2be0c013d0eddf4c9d2af29a71f05e21d7289808271d4c97db78
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -152,7 +152,8 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
}
}
- String newMetadataLocation = writeNewMetadata(metadata, currentVersion() + 1);
+ String newMetadataLocation = base == null && metadata.metadataFileLocation() != null ?
+ metadata.metadataFileLocation() : writeNewMetadata(metadata, currentVersion() + 1);
Review Comment:
4 spaces instead of 8 space indentation.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ @Test
+ public void testRegisterTableWithGivenBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"), false);
+ catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"),
+ "file:" + metadataVersionFiles.get(0));
+ Table newTable = catalog.loadTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"));
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"))).isTrue();
+ }
+
+ @Test
+ public void testRegisterTableBranchNotPresent() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
Review Comment:
please recheck the indentation in all newly added code.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ @Test
+ public void testRegisterTableWithGivenBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"), false);
+ catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"),
+ "file:" + metadataVersionFiles.get(0));
+ Table newTable = catalog.loadTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"));
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"))).isTrue();
+ }
+
+ @Test
+ public void testRegisterTableBranchNotPresent() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@default"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Nessie ref 'default' does not exist");
+ }
+
+ @Test
+ public void testRegisterTableWithDefaultBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TABLE_IDENTIFIER);
+ catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0));
+
+ Table newTable = catalog.loadTable(TABLE_IDENTIFIER);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
+ }
+
+ @Test
+ public void testRegisterTableTableAlreadyExists() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+
+ Assertions.assertThatThrownBy(() -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(AlreadyExistsException.class)
+ .hasMessage(
+ "Table already exists: db.tbl");
+ }
+
+ @Test
+ public void testRegisterTableTagName() throws NessieConflictException, NessieNotFoundException {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ api.createReference().sourceRefName(BRANCH).reference(Tag.of("tag_1", catalog.currentHash())).create();
+
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@tag_1"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("You can only mutate tables when using a branch without a hash or timestamp.");
+ }
+
+ @Ignore
+ public void testRegisterTableMoreThanOneBranch() throws NessieConflictException, NessieNotFoundException {
+ // Dependency on PR #5033 for the backend bug fixes in drop table.
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"), false);
+ catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"),
+ "file:" + metadataVersionFiles.get(0));
+ Table newTable = catalog.loadTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"));
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"))).isTrue();
+
+ api.createReference().sourceRefName(BRANCH).reference(Tag.of("branch1", catalog.currentHash())).create();
+
+ List<String> metadataVersionFiles1 = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles1.size());
+ catalog.dropTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@branch1`"));
+ catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@branch1`"),
+ "file:" + metadataVersionFiles1.get(0));
+ Table newTable1 = catalog.loadTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@branch1`"));
+ Assertions.assertThat(newTable1).isNotNull();
+ TableOperations ops1 = ((HasTableOperations) newTable1).operations();
+ String metadataLocation1 = ((NessieTableOperations) ops1).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles1.get(0)).isEqualTo(metadataLocation1);
+ Assertions.assertThat(catalog.tableExists(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@branch1`"))).isTrue();
+ }
+
private String getTableBasePath(String tableName) {
Review Comment:
Add a test case with invalid identifiers, non-null metadata path but metadata file not exist in that path, null metadata location.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ @Test
+ public void testRegisterTableWithGivenBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"), false);
Review Comment:
please validate the return values drop table, tablexist, registerTable by assert
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r901490027
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +361,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, List<String> metadataVersionFiles) {
+ Assertions.assertThat(catalog.dropTable(identifier, false)).isFalse();
+ Assertions.assertThat(catalog.registerTable(
+ identifier,
+ "file:" + metadataVersionFiles.get(0))).isNotNull();
+ Table newTable = catalog.loadTable(identifier);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(identifier)).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);
Review Comment:
No, that's not the case, it consumes identifier as one of the arguments which has been fed with "main", as the reference.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r909561421
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,81 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {
Review Comment:
I think we still miss:
- DynamoDbCatalog
- GlueCatalog
I do not like the duplicated test code, and I think it would be good to have a general Catalog API test for checking the functionality of the new Catalog implementations. Something like `TestCatalog` but for all/most of the catalog API methods. We can do it in a different PR, but it would be good to have them. The test for `registerTable()` is a good candidate for these common tests.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r901453932
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +361,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, List<String> metadataVersionFiles) {
+ Assertions.assertThat(catalog.dropTable(identifier, false)).isFalse();
+ Assertions.assertThat(catalog.registerTable(
+ identifier,
Review Comment:
Indentations are still not correct. Please recheck the newly added codes again.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +361,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, List<String> metadataVersionFiles) {
+ Assertions.assertThat(catalog.dropTable(identifier, false)).isFalse();
+ Assertions.assertThat(catalog.registerTable(
+ identifier,
+ "file:" + metadataVersionFiles.get(0))).isNotNull();
+ Table newTable = catalog.loadTable(identifier);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(identifier)).isTrue();
Review Comment:
load table is called above. So, this check is not needed as both are same functionality.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +361,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, List<String> metadataVersionFiles) {
+ Assertions.assertThat(catalog.dropTable(identifier, false)).isFalse();
+ Assertions.assertThat(catalog.registerTable(
+ identifier,
+ "file:" + metadataVersionFiles.get(0))).isNotNull();
+ Table newTable = catalog.loadTable(identifier);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(identifier)).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);
+ }
+
+ @Test
+ public void testRegisterTableNegativeScenarios() throws NessieConflictException, NessieNotFoundException {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ // Case 1: Branch does not exist
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@default"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Nessie ref 'default' does not exist");
+ // Case 2: Table Already Exists
+ Assertions.assertThatThrownBy(() -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(AlreadyExistsException.class)
+ .hasMessage(
+ "Table already exists: db.tbl");
+ // Case 3: Registering using a tag
+ api.createReference().sourceRefName(BRANCH).reference(Tag.of("tag_1", catalog.currentHash())).create();
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@tag_1"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("You can only mutate tables when using a branch without a hash or timestamp.");
+ // Case 4: non-null metadata path with null metadata location
+ Assertions.assertThat(catalog.dropTable(TABLE_IDENTIFIER, false)).isTrue();
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0) + "invalidName"))
+ .isInstanceOf(NotFoundException.class);
+ }
+
+ @Test
+ public void testRegisterTableWithDefaultBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ Assertions.assertThat(catalog.dropTable(TABLE_IDENTIFIER)).isTrue();
+ Assertions.assertThat(catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0))).isNotNull();
Review Comment:
suppose to reuse testRegister here?
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +361,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, List<String> metadataVersionFiles) {
+ Assertions.assertThat(catalog.dropTable(identifier, false)).isFalse();
+ Assertions.assertThat(catalog.registerTable(
+ identifier,
+ "file:" + metadataVersionFiles.get(0))).isNotNull();
+ Table newTable = catalog.loadTable(identifier);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(identifier)).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);
Review Comment:
testRegister has some validation, it was suppose to check against "main"?
I think it is checking with default branch.
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +361,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, List<String> metadataVersionFiles) {
+ Assertions.assertThat(catalog.dropTable(identifier, false)).isFalse();
+ Assertions.assertThat(catalog.registerTable(
+ identifier,
+ "file:" + metadataVersionFiles.get(0))).isNotNull();
+ Table newTable = catalog.loadTable(identifier);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(identifier)).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);
+ }
+
+ @Test
+ public void testRegisterTableNegativeScenarios() throws NessieConflictException, NessieNotFoundException {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ // Case 1: Branch does not exist
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@default"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Nessie ref 'default' does not exist");
+ // Case 2: Table Already Exists
+ Assertions.assertThatThrownBy(() -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(AlreadyExistsException.class)
+ .hasMessage(
+ "Table already exists: db.tbl");
+ // Case 3: Registering using a tag
+ api.createReference().sourceRefName(BRANCH).reference(Tag.of("tag_1", catalog.currentHash())).create();
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@tag_1"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("You can only mutate tables when using a branch without a hash or timestamp.");
+ // Case 4: non-null metadata path with null metadata location
+ Assertions.assertThat(catalog.dropTable(TABLE_IDENTIFIER, false)).isTrue();
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0) + "invalidName"))
+ .isInstanceOf(NotFoundException.class);
+ }
+
+ @Test
+ public void testRegisterTableWithDefaultBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ Assertions.assertThat(catalog.dropTable(TABLE_IDENTIFIER)).isTrue();
+ Assertions.assertThat(catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0))).isNotNull();
+
+ Table newTable = catalog.loadTable(TABLE_IDENTIFIER);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
+ }
+
+ @Ignore
+ public void testRegisterTableMoreThanOneBranch() throws NessieConflictException, NessieNotFoundException {
+ // Dependency on PR #5033 for the backend bug fixes in drop table.
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ catalog.dropTable(TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "@main`"), false);
Review Comment:
Test case is ignored but the comments of using TableReference and indentation etc are still applicable to this testcase
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r901451850
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +361,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, List<String> metadataVersionFiles) {
Review Comment:
better to accept single metadata file instead of list of files
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r902487356
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,81 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {
Review Comment:
Could we separate these tests for Nessie specific and general ones?
Could we run the "general" tests on every catalog, and move the `testRegisterTable`, and `testRegisterExistingTable` from `HiveTableTest` to this general place?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r902482676
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -196,7 +194,16 @@ public List<TableIdentifier> listTables(Namespace namespace) {
@Override
public boolean dropTable(TableIdentifier identifier, boolean purge) {
- return client.dropTable(identifier, purge);
+ TableReference tableReference = parseTableReference(identifier);
Review Comment:
Probably we should not mix PRs.
Let's commit the changes one-by-one
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] dimas-b commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
dimas-b commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r920179288
##########
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:
"Negative" in what sense? Would you mind renaming to `testRegisterTableFailureScenarios`? Also, it would be preferable to have a separate test method for each case, IMHO.
##########
aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java:
##########
@@ -295,6 +298,37 @@ public void testDropNamespace() {
Assert.assertFalse("namespace must not exist", response.hasItem());
}
+ @Test
+ public void testRegisterTable() {
+ Namespace namespace = Namespace.of(genRandomName());
Review Comment:
What is the purpose of using random names here? What do we achieve by randomization? Using random test input makes it harder to reproduce and debug failures :thinking:
##########
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) {
Review Comment:
This is a utility method, but it is names like a test method... Would you mind renaming to something like `validateRegister`?
##########
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 {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ // Case 1: Branch does not exist
+ ImmutableTableReference defaultTableReference =
+ ImmutableTableReference.builder().reference("default").name(TABLE_NAME).build();
+ TableIdentifier defaultIdentifier = TableIdentifier.of(DB_NAME, defaultTableReference.toString());
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ defaultIdentifier, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Nessie ref 'default' does not exist");
+ // Case 2: Table Already Exists
+ Assertions.assertThatThrownBy(() -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(AlreadyExistsException.class)
+ .hasMessage("Table already exists: db.tbl");
+ // Case 3: Registering using a tag
+ ImmutableTableReference branchTableReference =
+ ImmutableTableReference.builder().reference(BRANCH).name(TABLE_NAME).build();
+ TableIdentifier branchIdentifier = TableIdentifier.of(DB_NAME, branchTableReference.toString());
+ Assertions.assertThat(catalog.dropTable(branchIdentifier, false)).isTrue();
+ String hash = api.getReference().refName(BRANCH).get().getHash();
+ api.createReference().sourceRefName(BRANCH).reference(Tag.of("tag_1", hash)).create();
+ ImmutableTableReference tagTableReference =
+ ImmutableTableReference.builder().reference("tag_1").name(TABLE_NAME).build();
+ TableIdentifier tagIdentifier = TableIdentifier.of(DB_NAME, tagTableReference.toString());
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ tagIdentifier, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("You can only mutate tables when using a branch without a hash or timestamp.");
+ // Case 4: non-null metadata path with null metadata location
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0) + "invalidName"))
+ .isInstanceOf(NotFoundException.class);
+ // Case 5: null identifier
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(null, "file:" + metadataVersionFiles.get(0) + "invalidName"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid identifier: null");
+ }
+
+ @Test
+ public void testRegisterTableWithDefaultBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ Assertions.assertThat(catalog.dropTable(TABLE_IDENTIFIER, false)).isTrue();
+ testRegister(TABLE_IDENTIFIER, metadataVersionFiles.get(0));
+ }
+
+ @Test
+ public void testRegisterTableMoreThanOneBranch() {
Review Comment:
Where are other branches in this test? I can only see operations on `main` :thinking:
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieTableOperations.java:
##########
@@ -152,7 +152,8 @@ protected void doCommit(TableMetadata base, TableMetadata metadata) {
}
}
- String newMetadataLocation = writeNewMetadata(metadata, currentVersion() + 1);
+ String newMetadataLocation = (base == null) && (metadata.metadataFileLocation() != null) ?
+ metadata.metadataFileLocation() : writeNewMetadata(metadata, currentVersion() + 1);
Review Comment:
It looks like we're alternating between a simple get and a more involved write operation in this statement... Would you mind moving the write under an `if` to emphasize that it's a substantial operation?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r921105336
##########
aws/src/integration/java/org/apache/iceberg/aws/dynamodb/TestDynamoDbCatalog.java:
##########
@@ -295,6 +298,37 @@ public void testDropNamespace() {
Assert.assertFalse("namespace must not exist", response.hasItem());
}
+ @Test
+ public void testRegisterTable() {
+ Namespace namespace = Namespace.of(genRandomName());
Review Comment:
getRandomName() is similar to existing test cases, I have followed up on a similar line...
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r910178239
##########
core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java:
##########
@@ -65,6 +66,26 @@ public Table loadTable(TableIdentifier identifier) {
return result;
}
+ @Override
+ public org.apache.iceberg.Table registerTable(TableIdentifier identifier, String metadataFileLocation) {
Review Comment:
Can this be `Table` instead of the fully-qualified class name?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#issuecomment-1161522749
@pvary , @openinx, @szehon-ho, @RussellSpitzer , @rdblue : Can one of the maintainers please approve the first-time contributor flow for this PR?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary commented on pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
pvary commented on PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#issuecomment-1192350803
Merged the PR.
Thanks for the work @Mehul2500 and @ajantha-bhat for the review.
@Mehul2500 if you have time please go ahead with the CatalogAPI test PR.
Thanks,
Peter
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#issuecomment-1180028483
@pvary and @rdblue could you please review this PR.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] pvary merged pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
pvary merged PR #5037:
URL: https://github.com/apache/iceberg/pull/5037
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r901518715
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +361,107 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, List<String> metadataVersionFiles) {
+ Assertions.assertThat(catalog.dropTable(identifier, false)).isFalse();
+ Assertions.assertThat(catalog.registerTable(
+ identifier,
+ "file:" + metadataVersionFiles.get(0))).isNotNull();
+ Table newTable = catalog.loadTable(identifier);
+ Assertions.assertThat(newTable).isNotNull();
+ TableOperations ops = ((HasTableOperations) newTable).operations();
+ String metadataLocation = ((NessieTableOperations) ops).currentMetadataLocation();
+ Assertions.assertThat("file:" + metadataVersionFiles.get(0)).isEqualTo(metadataLocation);
+ Assertions.assertThat(catalog.tableExists(identifier)).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);
+ }
+
+ @Test
+ public void testRegisterTableNegativeScenarios() throws NessieConflictException, NessieNotFoundException {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ // Case 1: Branch does not exist
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@default"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Nessie ref 'default' does not exist");
+ // Case 2: Table Already Exists
+ Assertions.assertThatThrownBy(() -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(AlreadyExistsException.class)
+ .hasMessage(
+ "Table already exists: db.tbl");
+ // Case 3: Registering using a tag
+ api.createReference().sourceRefName(BRANCH).reference(Tag.of("tag_1", catalog.currentHash())).create();
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ TableIdentifier.of(DB_NAME, "`" + TABLE_NAME + "`@tag_1"),
+ "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("You can only mutate tables when using a branch without a hash or timestamp.");
+ // Case 4: non-null metadata path with null metadata location
+ Assertions.assertThat(catalog.dropTable(TABLE_IDENTIFIER, false)).isTrue();
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0) + "invalidName"))
+ .isInstanceOf(NotFoundException.class);
+ }
+
+ @Test
+ public void testRegisterTableWithDefaultBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ Assertions.assertThat(catalog.dropTable(TABLE_IDENTIFIER)).isTrue();
+ Assertions.assertThat(catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0))).isNotNull();
Review Comment:
please rebase on #5033
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r920345031
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,81 @@ public void testListTables() {
Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
}
+ private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {
Review Comment:
> I do not like the duplicated test code, and I think it would be good to have a general Catalog API test for checking the functionality of the new Catalog implementations. Something like TestCatalog but for all/most of the catalog API methods. We can do it in a different PR, but it would be good to have them. The test for registerTable() is a good candidate for these common tests.
We do have `CatalogTests`, which is what `TestRESTCatalog` is built off of. It’s nice in that it has some methods for subclasses to declare if the catalog being tested supports certain features, so individual tests can exit early if they don’t support an optional feature (similar to JUnit `Assume`).
For example if the catalog in question doesn’t support namespace properties.
Might be good to see if that can be made to fit this need or can otherwise get some ideas from it. I believe Nessie also implements these tests.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r921587564
##########
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:
updated...thank you
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Mehul2500 commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
Mehul2500 commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r921567555
##########
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 {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ // Case 1: Branch does not exist
+ ImmutableTableReference defaultTableReference =
+ ImmutableTableReference.builder().reference("default").name(TABLE_NAME).build();
+ TableIdentifier defaultIdentifier = TableIdentifier.of(DB_NAME, defaultTableReference.toString());
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ defaultIdentifier, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Nessie ref 'default' does not exist");
+ // Case 2: Table Already Exists
+ Assertions.assertThatThrownBy(() -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(AlreadyExistsException.class)
+ .hasMessage("Table already exists: db.tbl");
+ // Case 3: Registering using a tag
+ ImmutableTableReference branchTableReference =
+ ImmutableTableReference.builder().reference(BRANCH).name(TABLE_NAME).build();
+ TableIdentifier branchIdentifier = TableIdentifier.of(DB_NAME, branchTableReference.toString());
+ Assertions.assertThat(catalog.dropTable(branchIdentifier, false)).isTrue();
+ String hash = api.getReference().refName(BRANCH).get().getHash();
+ api.createReference().sourceRefName(BRANCH).reference(Tag.of("tag_1", hash)).create();
+ ImmutableTableReference tagTableReference =
+ ImmutableTableReference.builder().reference("tag_1").name(TABLE_NAME).build();
+ TableIdentifier tagIdentifier = TableIdentifier.of(DB_NAME, tagTableReference.toString());
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(
+ tagIdentifier, "file:" + metadataVersionFiles.get(0)))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("You can only mutate tables when using a branch without a hash or timestamp.");
+ // Case 4: non-null metadata path with null metadata location
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(TABLE_IDENTIFIER, "file:" + metadataVersionFiles.get(0) + "invalidName"))
+ .isInstanceOf(NotFoundException.class);
+ // Case 5: null identifier
+ Assertions.assertThatThrownBy(
+ () -> catalog.registerTable(null, "file:" + metadataVersionFiles.get(0) + "invalidName"))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid identifier: null");
+ }
+
+ @Test
+ public void testRegisterTableWithDefaultBranch() {
+ List<String> metadataVersionFiles = metadataVersionFiles(TABLE_NAME);
+ Assertions.assertThat(1).isEqualTo(metadataVersionFiles.size());
+ Assertions.assertThat(catalog.dropTable(TABLE_IDENTIFIER, false)).isTrue();
+ testRegister(TABLE_IDENTIFIER, metadataVersionFiles.get(0));
+ }
+
+ @Test
+ public void testRegisterTableMoreThanOneBranch() {
Review Comment:
One branch is "main", and another one is the default branch (i.e., iceberg-table-test), as I have not specified it, the second register table() function considers registration to the default branch.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ajantha-bhat commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()
Posted by GitBox <gi...@apache.org>.
ajantha-bhat commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r902491274
##########
nessie/src/main/java/org/apache/iceberg/nessie/NessieCatalog.java:
##########
@@ -196,7 +194,16 @@ public List<TableIdentifier> listTables(Namespace namespace) {
@Override
public boolean dropTable(TableIdentifier identifier, boolean purge) {
- return client.dropTable(identifier, purge);
+ TableReference tableReference = parseTableReference(identifier);
Review Comment:
@pvary: please help merging #5033, So that we can rebase this with master
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org