gh-yzou commented on code in PR #1231:
URL: https://github.com/apache/polaris/pull/1231#discussion_r2012980742
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/catalog/GenericTableCatalogTest.java:
##########
@@ -306,4 +336,166 @@ public void testGenericTableRoundTrip() {
Assertions.assertThat(resultEntity.getPropertiesAsMap()).isEqualTo(properties);
Assertions.assertThat(resultEntity.getName()).isEqualTo(tableName);
}
+
+ @Test
+ public void testLoadNonExistentTable() {
+ Namespace namespace = Namespace.of("ns");
+ icebergCatalog.createNamespace(namespace);
+
+ Assertions.assertThatCode(
+ () ->
genericTableCatalog.loadGenericTable(TableIdentifier.of("ns", "t1")))
+ .hasMessageContaining("does not exist: ns.t1");
+ }
+
+ @Test
+ public void testReadIcebergTableAsGeneric() {
+ Namespace namespace = Namespace.of("ns");
+ icebergCatalog.createNamespace(namespace);
+
+ String tableName = "t1";
+
+ icebergCatalog.createTable(TableIdentifier.of("ns", tableName), SCHEMA);
+ Assertions.assertThatCode(
+ () ->
genericTableCatalog.loadGenericTable(TableIdentifier.of("ns", tableName)))
+ .hasMessageContaining("does not exist: ns.t1");
+ }
+
+ @Test
+ public void testReadIcebergViewAsGeneric() {
+ Namespace namespace = Namespace.of("ns");
+ icebergCatalog.createNamespace(namespace);
+
+ String tableName = "t1";
+
+ icebergCatalog.buildView(TableIdentifier.of("ns", tableName));
+ Assertions.assertThatCode(
+ () ->
genericTableCatalog.loadGenericTable(TableIdentifier.of("ns", tableName)))
+ .hasMessageContaining("does not exist: ns.t1");
+ }
+
+ @Test
+ public void testReadGenericAsIcebergTable() {
+ Namespace namespace = Namespace.of("ns");
+ icebergCatalog.createNamespace(namespace);
+
+ String tableName = "t1";
+
+ genericTableCatalog.createGenericTable(TableIdentifier.of("ns",
tableName), "format", Map.of());
+ Assertions.assertThatCode(() ->
icebergCatalog.loadTable(TableIdentifier.of("ns", tableName)))
+ .hasMessageContaining("does not exist: ns.t1");
+ }
+
+ @Test
+ public void testReadGenericAsIcebergView() {
+ Namespace namespace = Namespace.of("ns");
+ icebergCatalog.createNamespace(namespace);
+
+ String tableName = "t1";
+
+ genericTableCatalog.createGenericTable(TableIdentifier.of("ns",
tableName), "format", Map.of());
+ Assertions.assertThatCode(() ->
icebergCatalog.loadView(TableIdentifier.of("ns", tableName)))
+ .hasMessageContaining("does not exist: ns.t1");
+ }
+
+ @Test
+ public void testListTables() {
+ Namespace namespace = Namespace.of("ns");
+ icebergCatalog.createNamespace(namespace);
+
+ for (int i = 0; i < 10; i++) {
+ genericTableCatalog.createGenericTable(TableIdentifier.of("ns", "t" +
i), "format", Map.of());
+ }
+
+ List<TableIdentifier> listResult =
genericTableCatalog.listGenericTables(namespace);
+
+ Assertions.assertThat(listResult.size()).isEqualTo(10);
+
Assertions.assertThat(listResult.stream().map(TableIdentifier::toString).toList())
+
.isEqualTo(listResult.stream().map(TableIdentifier::toString).sorted().toList());
+
+ Assertions.assertThat(icebergCatalog.listTables(namespace)).isEmpty();
+ }
+
+ @Test
+ public void testListTablesEmpty() {
+ Namespace namespace = Namespace.of("ns");
+ icebergCatalog.createNamespace(namespace);
+
+ for (int i = 0; i < 10; i++) {
+ icebergCatalog.createTable(TableIdentifier.of("ns", "t" + i), SCHEMA);
+ }
+
+
Assertions.assertThat(icebergCatalog.listTables(namespace).size()).isEqualTo(10);
+
Assertions.assertThat(genericTableCatalog.listGenericTables(namespace)).isEmpty();
+ }
+
+ @Test
+ public void testListIcebergTables() {
+ Namespace namespace = Namespace.of("ns");
+ icebergCatalog.createNamespace(namespace);
+
+ for (int i = 0; i < 10; i++) {
+ icebergCatalog.createTable(TableIdentifier.of("ns", "t" + i), SCHEMA);
+ }
+
+ List<TableIdentifier> listResult = icebergCatalog.listTables(namespace);
+
+ Assertions.assertThat(listResult.size()).isEqualTo(10);
+
Assertions.assertThat(listResult.stream().map(TableIdentifier::toString).toList())
+
.isEqualTo(listResult.stream().map(TableIdentifier::toString).sorted().toList());
+
+
Assertions.assertThat(genericTableCatalog.listGenericTables(namespace)).isEmpty();
+ }
+
+ @Test
+ public void testDropNonExistentTable() {
+ Namespace namespace = Namespace.of("ns");
+ icebergCatalog.createNamespace(namespace);
+
+ Assertions.assertThatCode(
+ () ->
genericTableCatalog.dropGenericTable(TableIdentifier.of("ns", "t1")))
+ .hasMessageContaining("Table does not exist: ns.t1");
+ }
+
+ @Test
+ public void testDropNonExistentNamespace() {
+ Namespace namespace = Namespace.of("ns");
+ icebergCatalog.createNamespace(namespace);
+
+ Assertions.assertThatCode(
+ () ->
genericTableCatalog.dropGenericTable(TableIdentifier.of("ns2", "t1")))
+ .hasMessageContaining("Table does not exist: ns2.t1");
+ }
+
+ @Test
+ public void testDropIcebergTable() {
+ Namespace namespace = Namespace.of("ns");
+ icebergCatalog.createNamespace(namespace);
+ icebergCatalog.createTable(TableIdentifier.of("ns", "t1"), SCHEMA);
+
+ Assertions.assertThatCode(
+ () ->
genericTableCatalog.dropGenericTable(TableIdentifier.of("ns", "t1")))
+ .hasMessageContaining("Table does not exist: ns.t1");
Review Comment:
sorry, i might be little bit confused here, this is the testDropIcebergTable
test point, right? I though the test is creating an iceberg table, and then we
are testing we can not drop the iceberg table through dropGenericTable. In that
case, we should be able to drop the iceberg table through icebergCatalog,
right? Or maybe I am missing something here?
But that is a nit suggestion here that the test could be more clear that the
Iceberg table can only be dropped through the Iceberg dropTable API not the
generic table API.
##########
service/common/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalog.java:
##########
@@ -1359,6 +1371,9 @@ public void doCommit(TableMetadata base, TableMetadata
metadata) {
.setMetadataLocation(newLocation)
.build();
}
+ if (entity.getType() == PolarisEntityType.GENERIC_TABLE) {
Review Comment:
nit: this is an suggestion, the logic is little bit hard to reason about
since all operations are done with IcebergTableLikeEntity, for example:
```
entity =
new IcebergTableLikeEntity.Builder(entity)
.setBaseLocation(metadata.location())
.setMetadataLocation(newLocation)
.build();
```
and then here we are checking whether the entity type is GENERIC_TABLE.
One way we can make this part of logic cleaner and still keep the same style
of code at both Catalog could be let the getTablePath function do two things:
1) validate if it conflicts with another another table entity that is not
the type we are looking for, for example, if an GENERIC_TABLE entity is found
when looking for iceberg table.
2) resolve the corresponding table entity if it is not conflicting with
another entity type.
For example, in the getTablePath function, we can do
```
if (icebergTableResult == null) {
PolarisResolvedPathWrapper genericTableResult
= resolvedEntityView.getPassthroughResolvedPath(
tableIdentifier, PolarisEntityType.GENERIC_TABLE,
PolarisEntitySubType.ANY_SUBTYPE);
if (genericTableResult != null &&
genericTableResult.getRawLeafEntity() != null) {
throw new AlreadyExistsException("Table already exists: %s",
identifier)
} else {
return null;
}
} else {
return icebergTableResult;
}
```
Then in that case, it is clear that any not null entity returned is an
IcebergTableLikeEntity, and goes through all iceberg entity required process.
We can do similar thing for the getTablePath function in GenericTableCatalog.
The function name could be updated properly 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
