Fetch all table/view names across schemas with one Hive metastore call (when Hive views enabled) #17127

huberty89 · 2023-04-19T16:42:51Z

Description

This PR replaces #16847 but drops last commits because of a regression - I will try to reintroduce that with a fix in separate PR.

Additional context and related issues

Release notes

( ) This is not user-visible or 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:

# General
* Fix some things. ({issue}`issuenumber`)

...trino-main/src/main/java/io/trino/connector/informationschema/InformationSchemaMetadata.java

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

...in/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreClient.java

...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/ForwardingHiveMetastore.java

...rino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastore.java

huberty89 · 2023-04-24T13:27:52Z

@findepi Could you take a look?

findepi · 2023-04-25T10:48:44Z

discussed offline

huberty89 · 2023-05-03T15:45:51Z

I feel I need to run TestHiveMetastoreMetadataQueriesAccessOperations for both cases:

HiveMetastore with implemented getAllTables() and getAllViews() - so this will cover ThriftMetastore
the backward compatible test - so those changes does not brake/slowdown for example GlueHiveMetastore

WDYT? @kokosing @Praveen2112 @findepi

huberty89 · 2023-05-04T10:12:40Z

Discussed offline

Praveen2112

Looks good. Need to run another round of review on TestHiveMetastoreMetadataQueriesAccessOperations

.../io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreMetadataQueriesAccessOperations.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

Praveen2112 · 2023-05-04T13:49:53Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/HiveMetastore.java

+    /**
+     * @return List of table names from all schemas or Optional.empty if operation is not supported
+     */
+    Optional<List<SchemaTableName>> getAllTables();


Is there is any specific reason for placing this below List<String> getAllTables(String databaseName); can we swap the places ?

Praveen2112 · 2023-05-04T14:37:50Z

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastore.java

@@ -1117,6 +1145,70 @@ public void testDropTable()
        assertEquals(mockClient.getAccessCount(), 5);
    }

+    @Test
+    public void testAllDatabases()


Can we extract it as a different commit.

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java

...rino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/MockThriftMetastoreClient.java

.../io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreMetadataQueriesAccessOperations.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/HiveMetastore.java

findepi · 2023-05-05T10:52:27Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/HiveMetastore.java

    List<String> getTablesWithParameter(String databaseName, String parameterKey, String parameterValue);

    List<String> getAllViews(String databaseName);

+    /**
+     * @return List of view names from all schemas or Optional.empty if operation is not supported


Can this include materialized views? let's document

findepi · 2023-05-05T10:54:26Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

+        // Without translateHiveViews, Hive views are represented as tables in Trino,
+        // and they should not be returned from ThriftHiveMetastore.getAllViews() call
+        if (!translateHiveViews) {
+            return Optional.empty();
+        }


we could remove this IF and adjust the code elsewhere.
We should follow-up on this.

.../trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java

.../io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreMetadataQueriesAccessOperations.java

findepi · 2023-05-05T11:01:46Z

the fact that TestHiveMetastoreMetadataQueriesAccessOperations.java is refactored in the commit introducing the improvement makes it hard to see what is the effect of the improvement on the invocation counts.

let's maybe introduce the dataprovider in TestHiveMetastoreMetadataQueriesAccessOperations.java as a prep commit (with some TODO / commit comment saying this is just a preparation step)

huberty89 · 2023-05-11T08:56:44Z

@findepi discussed offline: about filtering material views from views - this PR is only about performance improvement, I would not change current behavior even if it not correct - we can do it as a followup
cc @kokosing

.../io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreMetadataQueriesAccessOperations.java

So that they don't clash with subsequent commits new variables Co-authored-by: skrzypo987 <[email protected]>

Introduce trueFalse data provider so improvement will be easy to see.

Big calls listing metadata, like `SELECT * FROM information_schema.tables`, are common in BI tools, especially at the start. Trino then fetches all schemas and iterated over them to list tables and views. With n schemas it has to make 2*n calls to the underlying metastore. For a large number of schemas this may take several minutes if metastore communication overhead is big. This commit introduces a way to fetch all this metadata in one call, using `getTableMeta` thrift call. Other metastore implementations do not support this feature. This thrift call is hidden behind an alternative call. HiveMetastore interface now features two new methods - for fetching all tables and all views. The `CachingHiveMetastore` has two additional singleton cache objects containing full list of tables and views. Co-authored-by: skrzypo987 <[email protected]>

mosabua · 2023-05-17T22:08:34Z

@huberty89 and @findepi we had to invent our own release notes entry since none was suggested in the PR description or the release notes ticket or the release notes PR. Please provide more clarity in future PRs. We ended up with

## Hive connector

* Improve performance of querying `information_schema.tables` when using the
  Hive metastore. ({issue}`17127`)

However we are not clear if this applies to all other connectors that use the Hive metastore and therefore need to add this entry to Delta Lake, Iceberg and Hudi connector sections.

And we are also not sure about your mention about "when Hive views enabled" .. does that limit the scope to only the Hive connector after all?

cc @martint @colebow

huberty89 · 2023-05-18T10:45:36Z

@mosabua

However we are not clear if this applies to all other connectors that use the Hive metastore and therefore need to add this entry to Delta Lake, Iceberg and Hudi connector sections.

This optimization applies in Hive with HMS, for the rest of connectors which uses HMS I need to verify if further work is needed.

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java

hashhar · 2023-10-05T07:44:30Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

+        // Without translateHiveViews, Hive views are represented as tables in Trino,
+        // and they should not be returned from ThriftHiveMetastore.getAllViews() call
+        if (!translateHiveViews) {
+            return Optional.empty();
+        }


the impl of this method differs from getAllViews(String databaseName). that one seems to return tables with PRESTO_VIEW_FLAG set to true when translation is disabled instead of empty result.

was this intentional? Or am I misreading the code?

I think the initial implementation was supposed to work only when translateHiveViews or when !translateHiveViews (not sure which one).

cla-bot bot added the cla-signed label Apr 19, 2023

huberty89 requested review from findepi, kokosing and Praveen2112 April 19, 2023 17:21

github-actions bot added hive Hive connector tests:hive labels Apr 19, 2023

huberty89 force-pushed the huberty89/batch-get-table-in-thrift-metastore branch from c356de6 to b6c1819 Compare April 20, 2023 08:13

huberty89 requested a review from atanasenko April 20, 2023 10:04

kokosing reviewed Apr 20, 2023

View reviewed changes

huberty89 force-pushed the huberty89/batch-get-table-in-thrift-metastore branch 4 times, most recently from de9770a to 6c27aa7 Compare April 24, 2023 08:10

kokosing approved these changes Apr 24, 2023

View reviewed changes

huberty89 force-pushed the huberty89/batch-get-table-in-thrift-metastore branch from 6c27aa7 to 049acd8 Compare April 24, 2023 11:36

huberty89 mentioned this pull request Apr 26, 2023

Do not return all schema prefixes when there are more than 100 #17231

Merged

huberty89 force-pushed the huberty89/batch-get-table-in-thrift-metastore branch 2 times, most recently from de05a3b to 143d40c Compare May 2, 2023 15:19

huberty89 force-pushed the huberty89/batch-get-table-in-thrift-metastore branch from 143d40c to d3c6433 Compare May 4, 2023 11:11

Praveen2112 reviewed May 4, 2023

View reviewed changes

huberty89 force-pushed the huberty89/batch-get-table-in-thrift-metastore branch from d3c6433 to e5f6141 Compare May 5, 2023 09:09

findepi reviewed May 5, 2023

View reviewed changes

huberty89 force-pushed the huberty89/batch-get-table-in-thrift-metastore branch 2 times, most recently from eb807cc to 934fd05 Compare May 9, 2023 16:52

huberty89 force-pushed the huberty89/batch-get-table-in-thrift-metastore branch from 934fd05 to d15cdae Compare May 11, 2023 12:33

findepi approved these changes May 11, 2023

View reviewed changes

.../io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreMetadataQueriesAccessOperations.java Show resolved Hide resolved

findepi force-pushed the huberty89/batch-get-table-in-thrift-metastore branch from d15cdae to 9282667 Compare May 11, 2023 19:43

Hubert Łojek and others added 4 commits May 11, 2023 21:44

Make TestHiveMetastoreMetadataQueriesAccessOperations run faster

33cc583

Change variable names

0f313c6

So that they don't clash with subsequent commits new variables Co-authored-by: skrzypo987 <[email protected]>

Preparation step to improve readability of followup changes

f6579f7

Introduce trueFalse data provider so improvement will be easy to see.

findepi force-pushed the huberty89/batch-get-table-in-thrift-metastore branch from 9282667 to b44a4a5 Compare May 11, 2023 19:44

findepi approved these changes May 11, 2023

View reviewed changes

findepi merged commit d585eee into trinodb:master May 12, 2023

findepi mentioned this pull request May 12, 2023

information_schema.views fixes for materialized views #17444

Merged

github-actions bot added this to the 418 milestone May 12, 2023

colebow mentioned this pull request May 17, 2023

Add Trino 418 release notes #17540

Merged

heechan3006 mentioned this pull request Jul 3, 2023

Fetching all hive metadata failed issues #18111

Closed

huberty89 mentioned this pull request Jul 13, 2023

Add ability to disable batch fetch tables and views from HMS #18274

Merged

hashhar reviewed Oct 5, 2023

View reviewed changes

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java Show resolved Hide resolved

hashhar reviewed Oct 5, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch all table/view names across schemas with one Hive metastore call (when Hive views enabled) #17127

Fetch all table/view names across schemas with one Hive metastore call (when Hive views enabled) #17127

huberty89 commented Apr 19, 2023

huberty89 commented Apr 24, 2023

findepi commented Apr 25, 2023

huberty89 commented May 3, 2023

huberty89 commented May 4, 2023

Praveen2112 left a comment

Praveen2112 May 4, 2023

Praveen2112 May 4, 2023

findepi May 5, 2023

huberty89 May 11, 2023

findepi May 5, 2023

findepi commented May 5, 2023

huberty89 commented May 11, 2023

mosabua commented May 17, 2023 •

edited

Loading

huberty89 commented May 18, 2023 •

edited

Loading

hashhar Oct 5, 2023

findepi Oct 5, 2023

Fetch all table/view names across schemas with one Hive metastore call (when Hive views enabled) #17127

Fetch all table/view names across schemas with one Hive metastore call (when Hive views enabled) #17127

Conversation

huberty89 commented Apr 19, 2023

Description

Additional context and related issues

Release notes

huberty89 commented Apr 24, 2023

findepi commented Apr 25, 2023

huberty89 commented May 3, 2023

huberty89 commented May 4, 2023

Praveen2112 left a comment

Choose a reason for hiding this comment

Praveen2112 May 4, 2023

Choose a reason for hiding this comment

Praveen2112 May 4, 2023

Choose a reason for hiding this comment

findepi May 5, 2023

Choose a reason for hiding this comment

huberty89 May 11, 2023

Choose a reason for hiding this comment

findepi May 5, 2023

Choose a reason for hiding this comment

findepi commented May 5, 2023

huberty89 commented May 11, 2023

mosabua commented May 17, 2023 • edited Loading

huberty89 commented May 18, 2023 • edited Loading

hashhar Oct 5, 2023

Choose a reason for hiding this comment

findepi Oct 5, 2023

Choose a reason for hiding this comment

mosabua commented May 17, 2023 •

edited

Loading

huberty89 commented May 18, 2023 •

edited

Loading