Accelerate system.metadata.table_comments with Iceberg Glue catalog #18517

findepi · 2023-08-03T11:06:43Z

With Glue, table listing already pulls a lot information about tables.
For Trino-managed Iceberg tables this is sufficient information to
answer system.metadata.table_comments queries, without having to fetch
Glue tables again, one-by-one. Trino-manged Iceberg tables keep table
comment up to date in Glue (along with additional information sufficient
to verify it's indeed up to date). The approach can be generalized to
Iceberg's own GlueCatalog later if the community is interested.

Builds on #18315

release notes

# Iceberg
* Improve performance of `system.metadata.table_comments` when querying Iceberg tabes
  backed by Glue Catalog.

Apply whatever IntelliJ wants to have applied so that class reformat doesn't change the code.

Left over from unoptimized implementation of `testInformationSchemaColumns`.

core/trino-main/src/main/java/io/trino/connector/system/TableCommentSystemTable.java

core/trino-main/src/main/java/io/trino/metadata/Metadata.java

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

alexjo2144 · 2023-08-03T15:25:50Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java

+    {
+        if (prefix.getTable().isPresent()) {
+            // For single name referenced the default implementation is good enough
+            return ConnectorMetadata.super.streamRelationComments(session, prefix);


I'd personally prefer to continue passing the prefix's table down to the TrinoGlueCatalog implementation and have it handle this case. It's the same number of API calls as going back to the super's implementation, but only because of caching.

Similarly, do we have a test that exercises the path where both schema and table are provided in the prefix?

Similarly, do we have a test that exercises the path where both schema and table are provided in the prefix?

adding test for this case, search for // Pointed lookup.

but only because of caching.

i didn't want to optimize this path as i haven't seen this in the wild.

I couldn't get rid of reliance on caching because of how GlueCatalog and GlueTableOperations interact, so even the optimized path had this unfortunate dependency on the cache.

i don't want to pass prefix's down to the TrinoGlueCatalog implementation, to save complexity at least here. the biggest downside of this approach is complexity. some of the complexity seems unavoidable...

i decided to go into opposite direction. see adjust iceberg to Fixup simplify streamRelationComments: take Optional<String> schemaName instead of SchemaTablePrefix commit. I don't think tablePrefix abstraction buys us anything, and it just pushes more complexity into connectors.

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java

findepi · 2023-08-04T07:28:25Z

...c/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergGlueCatalogAccessOperations.java

@@ -545,8 +545,7 @@ public void testSystemMetadataTableComments()
                            session,
                            "SELECT * FROM system.metadata.table_comments WHERE schema_name = CURRENT_SCHEMA AND table_name LIKE 'test_select_s_m_t_comments%'",
                            ImmutableMultiset.<GlueMetastoreMethod>builder()
-                                    .addCopies(GET_TABLES, 3)
-                                    .addCopies(GET_TABLE, tables * 2)
+                                    .addCopies(GET_TABLES, 1)


- .addCopies(GET_TABLES, 3) - .addCopies(GET_TABLE, tables * 2) + .addCopies(GET_TABLES, 1)

looks like a nice change. thank you @ebyhr for your hint #18315 (review)
cc @kokosing @atanasenko @alexjo2144

By general rule, the engine should not call plugins asking for information that it can derive on its own. Here, engine should not need to call the plugin to filter empty list of entities.

findepi · 2023-08-04T14:30:32Z

TrinoGlueCatalog cleanup extracted to #18543.

alexjo2144 · 2023-08-04T18:55:27Z

core/trino-main/src/main/java/io/trino/connector/system/TableCommentSystemTable.java

+                }
+            }
+            else {
+                List<RelationCommentMetadata> relationComments = metadata.listRelationComments(


Should probably run access control checks again here in case the connector does not behave well in filtering out tables that should not be accessible.

I also saw there's an access control filterColumns method, do we need to be applying that here?

I also saw there's an access control filterColumns method, do we need to be applying that here?

i don't think it's applicable.

Should probably run access control checks again here in case the connector does not behave well in filtering out tables that should not be accessible.

if we want that, we would need to let the connector declare whether AC was consulted or not. otherwise we would be calling AC twice, which is redundant, and considerable cost (in case of many relations).

i had this in this PR, see before Fixup simplify streamRelationComments: require relationFilter to be applied commit. i didn't like the additional complexity though.

Note also that this is a matter of defining contract. We do not expect connectors to ignore schemaName parameter (tablePrefix in other methods). So we can choose not to expect connectors to ignore relationFilters.

alexjo2144

I think it's looking pretty close

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

alexjo2144 · 2023-08-04T19:05:07Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

+        return Optional.of(Stream.concat(
+                        unfilteredResultList.stream()
+                                .filter(commentMetadata -> availableNames.contains(commentMetadata.name())),
+                        filteredResult.build().stream())
+                .iterator());


We're doing a lot of eager work here. It doesn't matter yet because all the streams/iterators are collected to Lists farther up but if we switch the engine to do more of that lazily we'll want to come back to this.

agreed. however, i didn't see a clean way to to this somewhat non-trivial logic, where we want to filter tables after capturing comments for cases where we have comment information at hand, and we want to filter tables before going to file system. Doing it more lazy would mean we consult access control more times, which is going to be more expensive.
(previous table_comments implementation would gather even more state in-memory)

ebyhr · 2023-08-07T05:34:13Z

The result order differs before/after this change. I'm not sure whether the order was ensured, but it returned in alphabetical order as far as I confirmed. Can we keep the original order?

Order in DefaultIcebergQueryRunnerMain after this change:

 catalog_name |    schema_name     |    table_name    | comment
--------------+--------------------+------------------+---------
 tpch         | information_schema | tables           | NULL
 tpch         | information_schema | table_privileges | NULL
 tpch         | information_schema | schemata         | NULL
 tpch         | information_schema | columns          | NULL
 tpch         | information_schema | roles            | NULL
 tpch         | information_schema | enabled_roles    | NULL
 tpch         | information_schema | views            | NULL
 tpch         | information_schema | applicable_roles | NULL
 tpch         | sf1000             | supplier         | NULL
 tpch         | sf10000            | lineitem         | NULL
 tpch         | sf3000             | nation           | NULL
 tpch         | sf30000            | orders           | NULL
 tpch         | tiny               | orders           | NULL
 tpch         | tiny               | part             | NULL
 tpch         | sf30000            | part             | NULL
...

findepi · 2023-08-07T10:23:54Z

The result order differs before/after this change. I'm not sure whether the order was ensured, but it returned in alphabetical order as far as I confirmed. Can we keep the original order?

table_comments is a relation and relations do not define ordering over tuples they contain. In mathematical terms, an SQL relation is a multiset of tuples. The previously existing deterministic ordering was a side effect of listTables call being (apparently) ordered, but users should not rely on that. We could add explicit ordering, however (a) that would be unnecessary cost for users that do not need ordering, (b) that wouldn't free users from having explicit ORDER BY for cases where ordering is desired. I am tempted to ignore this aspect for now. @ebyhr wdyt?

findepi · 2023-08-07T10:45:55Z

(squashed and renamed (per #18517 (comment)))

ebyhr · 2023-08-08T06:26:45Z

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java

+    /**
+     * Gets comments for all relations (tables, views, materialized views) that match the specified prefix
+     * (e.g. for all relations that would be returned by {@link #listTables(ConnectorSession, Optional)} when
+     * {@code prefix} does not represent relation name).


This method doesn't take "prefix" as the method argument. Should we reword the sentence?

thanks, that's a left over. good catch!

Refactor implementation of the `system.metadata.table_comments` table to avoid`ConnectorMetadata.getTableMetadata` on per-table basis, which adds up and becomes expensive. This also allows avoiding separate listings for views and materialized views, and retrieval of unnecessary information (e.g. involving view text translation for Hive views).

With Glue, table listing already pulls a lot information about tables. For Trino-managed Iceberg tables this is sufficient information to answer `system.metadata.table_comments` queries, without having to fetch Glue tables again, one-by-one. Trino-manged Iceberg tables keep table comment up to date in Glue (along with additional information sufficient to verify it's indeed up to date). The approach can be generalized to Iceberg's own `GlueCatalog` later if the community is interested.

findepi · 2023-08-08T09:15:21Z

pushed javadoc fix (for #18517 (comment))
build green before push (https://github.com/trinodb/trino/actions/runs/5784144502)

findepi requested review from electrum, homar, Randgalt, losipiuk, ebyhr, alexjo2144 and findinpath August 3, 2023 11:06

cla-bot bot added the cla-signed label Aug 3, 2023

findepi added 2 commits August 3, 2023 13:07

Fix ConnectorMetadata javadoc formatting

c072c17

Apply whatever IntelliJ wants to have applied so that class reformat doesn't change the code.

Remove unused method

112a03a

Left over from unoptimized implementation of `testInformationSchemaColumns`.

findepi commented Aug 3, 2023

View reviewed changes

core/trino-main/src/main/java/io/trino/connector/system/TableCommentSystemTable.java Outdated Show resolved Hide resolved

findepi force-pushed the findepi/table-comments branch from 8a3ed54 to cca95db Compare August 3, 2023 11:09

github-actions bot added the iceberg Iceberg connector label Aug 3, 2023

findepi force-pushed the findepi/table-comments branch from cca95db to ce1caf9 Compare August 3, 2023 14:31

findepi mentioned this pull request Aug 3, 2023

System tables optimization #18515

Draft

alexjo2144 reviewed Aug 3, 2023

View reviewed changes

findepi commented Aug 3, 2023

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java Show resolved Hide resolved

core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java Outdated Show resolved Hide resolved

findepi commented Aug 4, 2023

View reviewed changes

findepi added 2 commits August 4, 2023 12:55

Short-circuit filter* AccessControl methods

30ab285

By general rule, the engine should not call plugins asking for information that it can derive on its own. Here, engine should not need to call the plugin to filter empty list of entities.

Test system.metadata.table_comments cost with Iceberg

6d3f7b3

findepi force-pushed the findepi/table-comments branch from ce1caf9 to 0153dfb Compare August 4, 2023 11:39

findepi force-pushed the findepi/table-comments branch 2 times, most recently from 503b8ff to b633c4e Compare August 4, 2023 15:16

alexjo2144 reviewed Aug 4, 2023

View reviewed changes

findepi force-pushed the findepi/table-comments branch from 73edb2e to 67ed8fd Compare August 7, 2023 10:45

ebyhr approved these changes Aug 8, 2023

View reviewed changes

findepi added 2 commits August 8, 2023 11:14

findepi force-pushed the findepi/table-comments branch from 67ed8fd to afe1bd5 Compare August 8, 2023 09:14

findepi merged commit 69f8d74 into master Aug 8, 2023

findepi deleted the findepi/table-comments branch August 8, 2023 09:26

github-actions bot added this to the 423 milestone Aug 8, 2023

mosabua mentioned this pull request Aug 8, 2023

Add Trino 423 release notes #18496

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accelerate system.metadata.table_comments with Iceberg Glue catalog #18517

Accelerate system.metadata.table_comments with Iceberg Glue catalog #18517

findepi commented Aug 3, 2023 •

edited

Loading

alexjo2144 Aug 3, 2023

alexjo2144 Aug 3, 2023

findepi Aug 4, 2023

findepi Aug 4, 2023

findepi Aug 4, 2023

findepi commented Aug 4, 2023

alexjo2144 Aug 4, 2023

findepi Aug 4, 2023

alexjo2144 left a comment

alexjo2144 Aug 4, 2023

findepi Aug 4, 2023

ebyhr commented Aug 7, 2023 •

edited

Loading

findepi commented Aug 7, 2023

findepi commented Aug 7, 2023

ebyhr Aug 8, 2023

findepi Aug 8, 2023

findepi commented Aug 8, 2023

Accelerate system.metadata.table_comments with Iceberg Glue catalog #18517

Accelerate system.metadata.table_comments with Iceberg Glue catalog #18517

Conversation

findepi commented Aug 3, 2023 • edited Loading

release notes

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

findepi commented Aug 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjo2144 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr commented Aug 7, 2023 • edited Loading

findepi commented Aug 7, 2023

findepi commented Aug 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Aug 8, 2023

findepi commented Aug 3, 2023 •

edited

Loading

ebyhr commented Aug 7, 2023 •

edited

Loading