TrinoHiveCatalog listTables should list all tables #11617
Conversation
alexjo2144
force-pushed
the
iceberg/list-tables
branch
from
c9ab073 to
10d58f6
Compare
| @@ -286,22 +281,9 @@ public Transaction newCreateTableTransaction( | |||
| public List<SchemaTableName> listTables(ConnectorSession session, Optional<String> namespace) | |||
| { | |||
| ImmutableSet.Builder<SchemaTableName> tablesListBuilder = ImmutableSet.builder(); | |||
| listNamespaces(session, namespace) | |||
There was a problem hiding this comment.
For context, this is the initial commit which added the filtering of Iceberg tables in the listing:
There was a problem hiding this comment.
Even farther back #1354
|
What is expected actually from a functional point of view when saying "show tables" in hive/iceberg/delta? |
The latter is preferred, but not always possible do achieve in a performant manner. The former is acceptable. In case of redirections, this should be "all tables which can be read by a user" => all tables (reasonably good approximation). |
...t-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergHiveTablesCompatibility.java
Outdated
Show resolved
Hide resolved
alexjo2144
force-pushed
the
iceberg/list-tables
branch
from
10d58f6 to
f1c269d
Compare
| @@ -65,4 +67,22 @@ public void testHiveSelectFromIcebergTable() | |||
|
|
|||
| onTrino().executeQuery("DROP TABLE iceberg.default." + tableName); | |||
| } | |||
|
|
|||
| @Test(groups = {ICEBERG, STORAGE_FORMATS, HMS_ONLY}) | |||
There was a problem hiding this comment.
Please create a follow-up to add eventually the same tests for hive-delta duo.
alexjo2144
force-pushed
the
iceberg/list-tables
branch
from
f1c269d to
9f3614f
Compare
|
AC and resolved merge conflicts |
...oduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergHiveMetadataListing.java
Show resolved
Hide resolved
| // TODO: support redirects from Iceberg to Hive | ||
| assertThat(readTableComment("iceberg", "default", tableName)).hasNoRows(); |
There was a problem hiding this comment.
Looks like this fixes a (small) bug, right?
There was a problem hiding this comment.
Without redirections hiding the non-Iceberg tables could have been nice to have, at the cost of making drop schema sometimes fail in confusing ways
Once we have redirections, hiding these tables would certainly be a bug
please rebase again, since there are new merge conflicts. sorry |
alexjo2144
force-pushed
the
iceberg/list-tables
branch
from
9f3614f to
b6a63e7
Compare
|
Rebased again, thanks |
Excluding non-Iceberg tables makes a schema seem empty when it is not.
alexjo2144
force-pushed
the
iceberg/list-tables
branch
from
b6a63e7 to
1e6fd96
Compare
Description
Excluding non-Iceberg tables makes a schema seem empty
when it is not.
Revert of #1354
One situation that is confusing right now is dropping a schema that has only hive tables in it.
DROP SCHEMA iceberg.my_schemawill fail because the schema is not empty (in that case the exception comes from the HMS, not Trino because Trino thinks the schema is empty). ASHOW TABLES FROM iceberg.my_schemawould come back empty.it'll also be relevant for table redirections #11356. In order to redirect a table, the iceberg catalog needs to know it exists.
This also brings the iceberg connector in line with the Hive connector. Listing shows all available tables in the HMS.
Fix
Iceberg Connector with HMS
Related issues, pull requests, and links
Documentation
(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.
Release notes
( ) No release notes entries required.
(x) Release notes entries required with the following suggested text: