wombatu-kun commented on code in PR #16891:
URL: https://github.com/apache/iceberg/pull/16891#discussion_r3463918748
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/SparkCatalog.java:
##########
@@ -368,6 +369,31 @@ public Identifier[] listTables(String[] namespace) {
.toArray(Identifier[]::new);
}
+ @Override
+ public TableSummary[] listTableSummaries(String[] namespace) {
+ // Build summaries directly from the catalog listings to avoid loading
every table, which the
+ // default TableCatalog.listTableSummaries implementation would do.
Iceberg tables are always
+ // reported as EXTERNAL (see SparkTable#properties), and views are
reported as VIEW.
Review Comment:
`properties()` is declared on `BaseSparkTable`, not `SparkTable` (which
inherits it without overriding). Reference `BaseSparkTable#properties` so the
pointer lands on where the EXTERNAL `table_type` is actually set.
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/TestSparkCatalogOperations.java:
##########
@@ -122,6 +128,81 @@ public void testAlterTable() throws NoSuchTableException {
.containsEntry(propsKey, propsValue);
}
+ @TestTemplate
+ public void testTableTypeIsExternal() throws NoSuchTableException {
+ BaseCatalog catalog = (BaseCatalog)
spark.sessionState().catalogManager().catalog(catalogName);
+ Identifier identifier = Identifier.of(tableIdent.namespace().levels(),
tableIdent.name());
+
+ Table table = catalog.loadTable(identifier);
+
+ assertThat(table.properties())
+ .as("Iceberg tables should report an EXTERNAL table type")
+ .containsEntry(TableCatalog.PROP_TABLE_TYPE,
TableSummary.EXTERNAL_TABLE_TYPE);
+ }
+
+ @TestTemplate
+ public void testListTableSummaries() throws NoSuchNamespaceException,
NoSuchTableException {
Review Comment:
This test's EXTERNAL-table assertion duplicates the one
`testListTableSummariesIncludesViews` already makes. The only coverage it adds
is running on the HADOOP and SPARK_SESSION params, which that test skips via
`assumeThat`. Consider folding the two by gating only the view-specific
assertions on a `ViewCatalog`, so the EXTERNAL-table check still runs on all
params, then dropping this separate test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
