Add iceberg_tables table function #24469

lukasz-stec · 2024-12-13T10:49:12Z

Description

Add system.iceberg_tables table function

Add the ability to list only iceberg tables from the iceberg catalog.
Before this change, there was no way to list only iceberg tables.
The SHOW TABLES statement, information_schema.tables, and jdbc.tables will all
return all tables that exist in the underlying metastore, even if the table cannot
be handled in any way by the iceberg connector. This can happen if other connectors
like hive or delta, use the same metastore, catalog, and schema to store its tables.
The function accepts an optional parameter with the schema name.
Sample statements:

SELECT * FROM TABLE(iceberg.system.iceberg_tables());
SELECT * FROM TABLE(iceberg.system.iceberg_tables(SCHEMA_NAME => 'test'));

Additional context and related issues

#11617 removed the non-iceberg table filter from TrinoHiveCatalog
#21114 removed the getTablesWithParameter metastore API that is re-added here

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(X ) Release notes are required, with the following suggested text:

## Iceberg connector
* Add `system.iceberg_tables` table function to the iceberg connector to allow listing only iceberg tables

raunaqmorarka

lgtm % comments

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java

lib/trino-metastore/src/main/java/io/trino/metastore/HiveMetastore.java

lukasz-stec

CA

lib/trino-metastore/src/main/java/io/trino/metastore/HiveMetastore.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/cache/CachingHiveMetastore.java

lib/trino-metastore/src/main/java/io/trino/metastore/HiveMetastore.java

raunaqmorarka · 2024-12-18T07:14:06Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java

+    {
+        requireNonNull(parameterKey, "parameterKey is null");
+        requireNonNull(parameterValues, "parameterValue is null");
+        return doListAllTables(databaseName, table -> parameterValues.contains(table.getParameters().get(parameterKey))).stream()


Do we need to handle the case where table.getParameters().get(parameterKey) is null separately ? That potentially is nullptr exception

I changed the type of the parameterValues to ImmutableSet to avoid this problem.

I'm a bit confused about how changing the type helps.
I was thinking about the case where given parameter key does not exist in table parameters. I think the safe thing to do would be to return empty list of table names in that case.

A Set can contain null values so theoretically an API with Set could mean a caller would like to filter for null or non-existent values. ImmutableSet cannot contain null, and additionally, it is safe to check for null existence there. This means that ImmutableSet better describes the API here which only supports non-null values for the filter

That seems a subtle distinction, it would be easy for users of the API to miss it.
Can we just handle that corner case safely instead ?

How can they miss it? With ImmutableSet there is no way to pass the wrong argument

We can't pass in a key for which there is no entry in table parameters ?

They can, that is actually the point of this to return only those that have it. But that works ok becasue the parameters values cannot contain null thus potential mistake to try to filter by null value will just not work and it is safe to check for null element becasue ImmutableSet as all immutable collections explicitly allows for null argument to com.google.common.collect.ImmutableCollection#contains.

public abstract boolean contains(@CheckForNull Object object);

raunaqmorarka · 2024-12-18T07:18:41Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/glue/GlueHiveMetastore.java

+        return getTablesInternal(
+                _ -> {},
+                databaseName,
+                table -> table.parameters() != null && parameterValues.contains(table.parameters().get(parameterKey))).stream()


Do we need to handle the case where table.parameters().get(parameterKey) is null separately ? That potentially is nullptr exception

lib/trino-metastore/src/main/java/io/trino/metastore/HiveMetastore.java

...o-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/snowflake/TrinoSnowflakeCatalog.java

...no-iceberg/src/main/java/io/trino/plugin/iceberg/functions/tables/IcebergTablesFunction.java

...n/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/IcebergFunctionProvider.java

lukasz-stec

CA

lib/trino-metastore/src/main/java/io/trino/metastore/HiveMetastore.java

lukasz-stec · 2024-12-18T09:09:34Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java

+    {
+        requireNonNull(parameterKey, "parameterKey is null");
+        requireNonNull(parameterValues, "parameterValue is null");
+        return doListAllTables(databaseName, table -> parameterValues.contains(table.getParameters().get(parameterKey))).stream()


I changed the type of the parameterValues to ImmutableSet to avoid this problem.

...no-iceberg/src/main/java/io/trino/plugin/iceberg/functions/tables/IcebergTablesFunction.java

lib/trino-metastore/src/main/java/io/trino/metastore/HiveMetastore.java

...o-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/snowflake/TrinoSnowflakeCatalog.java

...n/trino-iceberg/src/main/java/io/trino/plugin/iceberg/functions/IcebergFunctionProvider.java

raunaqmorarka

please resolve conflicts

The dropMaterializedView on TrinoHiveCatalog that uses FileHiveMetastore works only if unique table locations are enabled.

TrinoCatalog.listTables returns not only iceberg tables but also other relations like views, materialized view or non-iceberg tables.

lukasz-stec · 2024-12-20T12:02:59Z

please resolve conflicts

done

Add the ability to list only iceberg tables from the iceberg catalog. Before this change, there was no way to list only iceberg tables. The SHOW TABLES statement, information_schema.tables, and jdbc.tables will all return all tables that exist in the underlying metastore, even if the table cannot be handled in any way by the iceberg connector. This can happen if other connectors like hive or delta, use the same metastore, catalog, and schema to store its tables. The function accepts an optional parameter with the schema name. Sample statements: SELECT * FROM TABLE(iceberg.system.iceberg_tables()); SELECT * FROM TABLE(iceberg.system.iceberg_tables(SCHEMA_NAME => 'test'));

raunaqmorarka · 2024-12-23T08:32:55Z

Please work on adding documentation as a follow-up

ebyhr · 2024-12-23T09:17:20Z

This PR should have gotten syntax reviews from @martint as I talked with @raunaqmorarka offline.

@lukasz-stec Could you check CI failures on master? https://github.com/trinodb/trino/actions/runs/12463782682/job/34786781881

lukasz-stec · 2024-12-23T09:59:52Z

@lukasz-stec Could you check CI failures on master? https://github.com/trinodb/trino/actions/runs/12463782682/job/34786781881

Those tests were not run as part of this PR, missing label probably. I will need to fix them.

lukasz-stec · 2024-12-23T10:37:06Z

@lukasz-stec Could you check CI failures on master? https://github.com/trinodb/trino/actions/runs/12463782682/job/34786781881

Those tests were not run as part of this PR, missing label probably. I will need to fix them.

I created #24561. @ebyhr @raunaqmorarka PTAL + I think it needs to run with secrets as from what I understand this was the reason cloud tests were not run in this PR

cla-bot bot added the cla-signed label Dec 13, 2024

github-actions bot added iceberg Iceberg connector hive Hive connector labels Dec 13, 2024

lukasz-stec changed the title ~~Ls/2412/04 iceberg tables function~~ Iceberg tables table function Dec 13, 2024

lukasz-stec changed the title ~~Iceberg tables table function~~ Add iceberg_tables table function Dec 13, 2024

ebyhr added the syntax-needs-review label Dec 13, 2024

lukasz-stec force-pushed the ls/2412/04-iceberg-tables-function branch 2 times, most recently from 8d77152 to 10160e7 Compare December 13, 2024 15:10

lukasz-stec marked this pull request as ready for review December 13, 2024 19:17

lukasz-stec requested review from dain, electrum, raunaqmorarka, ebyhr and alexjo2144 December 13, 2024 19:17

raunaqmorarka reviewed Dec 16, 2024

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/TrinoCatalog.java Outdated Show resolved Hide resolved

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorSmokeTest.java Show resolved Hide resolved

oskar-szwajkowski reviewed Dec 16, 2024

View reviewed changes

lib/trino-metastore/src/main/java/io/trino/metastore/HiveMetastore.java Outdated Show resolved Hide resolved

lukasz-stec force-pushed the ls/2412/04-iceberg-tables-function branch from 73ae38d to aca9303 Compare December 17, 2024 09:35

lukasz-stec commented Dec 17, 2024

View reviewed changes

lukasz-stec requested review from raunaqmorarka and oskar-szwajkowski December 17, 2024 09:36

raunaqmorarka reviewed Dec 18, 2024

View reviewed changes

lukasz-stec force-pushed the ls/2412/04-iceberg-tables-function branch from aca9303 to a31b92c Compare December 18, 2024 18:22

lukasz-stec commented Dec 18, 2024

View reviewed changes

lukasz-stec requested a review from raunaqmorarka December 18, 2024 18:22

lukasz-stec force-pushed the ls/2412/04-iceberg-tables-function branch from a31b92c to 44a1c84 Compare December 18, 2024 20:52

raunaqmorarka approved these changes Dec 20, 2024

View reviewed changes

lukasz-stec added 4 commits December 20, 2024 12:56

Add tests for dropMaterializedView

06a56c5

The dropMaterializedView on TrinoHiveCatalog that uses FileHiveMetastore works only if unique table locations are enabled.

Replace testView method override with getViewType

e9c9e3d

Extend testListTables with other relation types

f703559

TrinoCatalog.listTables returns not only iceberg tables but also other relations like views, materialized view or non-iceberg tables.

Add HiveMetastore.getTableNamesWithParameters

2060f23

Add TrinoCatalog.listIcebergTables

6e11f5a

lukasz-stec force-pushed the ls/2412/04-iceberg-tables-function branch from 44a1c84 to bfeae5e Compare December 20, 2024 12:02

lukasz-stec force-pushed the ls/2412/04-iceberg-tables-function branch from bfeae5e to 18647ba Compare December 20, 2024 12:44

empty

ca9687e

raunaqmorarka merged commit 5ce80be into trinodb:master Dec 23, 2024
62 checks passed

raunaqmorarka deleted the ls/2412/04-iceberg-tables-function branch December 23, 2024 08:32

github-actions bot added this to the 469 milestone Dec 23, 2024

lukasz-stec mentioned this pull request Dec 23, 2024

Fix failures in iceberg cloud tests #24561

Merged

ebyhr mentioned this pull request Dec 23, 2024

Revert "Add iceberg_tables table function" #24565

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add iceberg_tables table function #24469

Add iceberg_tables table function #24469

lukasz-stec commented Dec 13, 2024

raunaqmorarka left a comment

lukasz-stec left a comment

raunaqmorarka Dec 18, 2024

lukasz-stec Dec 18, 2024

raunaqmorarka Dec 19, 2024

lukasz-stec Dec 19, 2024

raunaqmorarka Dec 19, 2024

lukasz-stec Dec 19, 2024

raunaqmorarka Dec 19, 2024

lukasz-stec Dec 19, 2024

raunaqmorarka Dec 18, 2024

lukasz-stec left a comment

lukasz-stec Dec 18, 2024

raunaqmorarka left a comment

lukasz-stec commented Dec 20, 2024

raunaqmorarka commented Dec 23, 2024

ebyhr commented Dec 23, 2024

lukasz-stec commented Dec 23, 2024

lukasz-stec commented Dec 23, 2024

Add iceberg_tables table function #24469

Add iceberg_tables table function #24469

Conversation

lukasz-stec commented Dec 13, 2024

Description

Additional context and related issues

Release notes

raunaqmorarka left a comment

Choose a reason for hiding this comment

lukasz-stec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-stec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raunaqmorarka left a comment

Choose a reason for hiding this comment

lukasz-stec commented Dec 20, 2024

raunaqmorarka commented Dec 23, 2024

ebyhr commented Dec 23, 2024

lukasz-stec commented Dec 23, 2024

lukasz-stec commented Dec 23, 2024