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

huberty89 · 2023-04-25T15:17:27Z

Description

Query:

SELECT * FROM information_schema.tables WHERE table_name LIKE 'test%'

fails when there is more than 100 schemas.

Additional context and related issues

Fix small regression from #17121

Release notes

(x) 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.
( ) Release notes are required, with the following suggested text:

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

...c/test/java/io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreReadAccessOperations.java

findepi · 2023-04-26T07:10:34Z

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

+        if (schemaPrefixes.size() > MAX_PREFIXES_COUNT) {
+            // in case of high number of prefixes it is better to populate all data and then filter
+            // TODO this may cause re-running the above filtering upon next applyFilter
+            return defaultPrefixes(catalogName);


This probably fixes a bug I introduced in 677b52d.
calculatePrefixesWithSchemaName could return many prefixes
then calculatePrefixesWithTableName could exit here

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

Line 312 in 1474245

return prefixes;

and then this verification would fail

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

Line 222 in 1474245

verify(tablePrefixes.size() <= MAX_PREFIXES_COUNT, "calculatePrefixesWithTableName returned too many prefixes: %s", tablePrefixes.size());

Can you call the commit "Fix ..." if indeed this is a bug fix?
is it worth adding a test?

testSelectTablesWithLikeOverSchema and testSelectTablesWithLikeOverTableName from added TestHiveMetastoreReadAccessOperations detect that case.

If you would reorder commits then it would be easily noticed that it is an improvement.

Renamed commit message with short explanation

@huberty89 thanks for adding

When there is more than 100 schemas InformationSchemaMetadata#calculatePrefixesWithSchemaName should return default prefix instead of all schemas

did the code fail before?
locally see that the new test io.trino.plugin.hive.metastore.thrift.TestHiveMetastoreReadAccessOperations#testSelectTablesWithLikeOverSchema would fail without this change, so i am marking this PR as a release blocker

Yes, it was failing - exception from verify check.

btw thanks for fixing my bug

Happy to help

plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/CountingAccessHiveMetastore.java

findepi · 2023-04-26T07:11:32Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/CountingAccessHiveMetastore.java

+        methodInvocations.add(Methods.GET_ALL_TABLES_WITH_PARAMETER);
+        return delegate.getAllTables(databaseName);


why _WITH_PARAMETER? shouldn't this be GET_ALL_TABLES?

In PR #17127 I will add getAllTables without any parameter

...c/test/java/io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreReadAccessOperations.java

findepi · 2023-04-26T07:16:43Z

...c/test/java/io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreReadAccessOperations.java

+        queryRunner.installPlugin(new TestingHivePlugin(metastore));
+        queryRunner.createCatalog("hive", "hive", ImmutableMap.of());
+
+        for (int i = 0; i <= MAX_PREFIXES_COUNT; i++) {


i -> schemaNumber
< TEST_SCHEMAS_COUNT

...c/test/java/io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreReadAccessOperations.java

kokosing

Thank you. This test is nice.

kokosing · 2023-04-26T12:58:12Z

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

+        if (schemaPrefixes.size() > MAX_PREFIXES_COUNT) {
+            // in case of high number of prefixes it is better to populate all data and then filter
+            // TODO this may cause re-running the above filtering upon next applyFilter
+            return defaultPrefixes(catalogName);


If you would reorder commits then it would be easily noticed that it is an improvement.

kokosing · 2023-04-26T12:59:15Z

plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/CountingAccessHiveMetastore.java

@@ -341,6 +344,7 @@ public void updatePartitionStatistics(Table table, Map<String, Function<Partitio
    @Override
    public List<String> getAllTables(String databaseName)
    {
-        throw new UnsupportedOperationException();
+        methodInvocations.add(Methods.GET_ALL_TABLES_WITH_PARAMETER);


nit: static import

...c/test/java/io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreReadAccessOperations.java

kokosing · 2023-04-26T13:02:47Z

...c/test/java/io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreReadAccessOperations.java

+import static io.trino.plugin.hive.metastore.file.FileHiveMetastore.createTestingFileHiveMetastore;
+import static io.trino.testing.TestingSession.testSessionBuilder;
+
+@Test(singleThreaded = true) // metastore invocation counters shares mutable state so can't be run from many threads simultaneously


Please copy-paste tests but for system.jdbc. To capture how things looks today and prevent any regression. Please do not fix any encountered problem as part of this PR.

Done but I feel we should use trino-jdbc to have exact queries used by JDBC

system is also a plain connector so the fact it is used in jdbc is not that much important. We need are testing a system connector integration with hive connector here. Putting JDBC into the same test sounds like an overkill as it is already a bit complex.

we already exercise system.jdbc directly in other tests (see eg BCT).
better to do that directly than not do that at all.

testing via JDBC could be more verbose, i wouldn't insist on that, but wouldn't also oppose strongly.

system is also a plain connector so the fact it is used in jdbc is not that much important.

i don't agree.
system.jdbc is supposed to be accessed via JDBC and we care primarily about usage patterns generated by the JDBC driver.
at this point, i wouldn't bother about performance of, say, joins or union queries or anything like that on top of system.jdbc, because it's outside of typical usage pattern

When there is more than 100 schemas InformationSchemaMetadata#calculatePrefixesWithSchemaName should return default prefix instead of all schemas

...c/test/java/io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreReadAccessOperations.java

findepi · 2023-04-26T13:25:09Z

release-blocker per #17231 (comment)
(regression introduced in #17121)

...o/plugin/hive/metastore/thrift/AbstractTestHiveMetastoreMetadataQueriesAccessOperations.java

...hive/metastore/thrift/TestHiveMetastoreInformationSchemaMetadataQueriesAccessOperations.java

Praveen2112 · 2023-04-27T07:59:52Z

...hive/metastore/thrift/TestHiveMetastoreInformationSchemaMetadataQueriesAccessOperations.java

+    @Test
+    public void testSelectColumnsWithLikeOverTableName()
+    {
+        assertMetastoreInvocations("SELECT * FROM information_schema.columns WHERE table_name LIKE 'test%'",


What if the like pattern matches a single table ? Does it still be non-deterministic ?

Praveen2112 · 2023-04-27T08:04:24Z

...trino/plugin/hive/metastore/thrift/TestHiveMetastoreJdbcMetadataQueriesAccessOperations.java

+import static io.trino.plugin.hive.metastore.CountingAccessHiveMetastore.Methods.GET_TABLE;
+
+@Test(singleThreaded = true) // metastore invocation counters shares mutable state so can't be run from many threads simultaneously
+public class TestHiveMetastoreJdbcMetadataQueriesAccessOperations


nit: Is it possible to move the test and assertion to the base class - if the assertions are same ? We could expose some sort of an API - that could expose the tableName and a columnName which could be overridden for InformationSchema or system.jdbc

When I merge both implementation together it's much easier to read which implementation (information_schema or JDBC) lagging behind

kokosing · 2023-04-27T09:45:30Z

...o/plugin/hive/metastore/thrift/AbstractTestHiveMetastoreMetadataQueriesAccessOperations.java

+import static io.trino.plugin.hive.metastore.file.FileHiveMetastore.createTestingFileHiveMetastore;
+import static io.trino.testing.TestingSession.testSessionBuilder;
+
+public class AbstractTestHiveMetastoreMetadataQueriesAccessOperations


BaseHiveMetastoreMetadataQueriesAccessOperationsTest

this class will be merged back

Merged together

kokosing · 2023-04-27T09:46:27Z

...trino/plugin/hive/metastore/thrift/TestHiveMetastoreJdbcMetadataQueriesAccessOperations.java

+import static io.trino.plugin.hive.metastore.CountingAccessHiveMetastore.Methods.GET_TABLE;
+
+@Test(singleThreaded = true) // metastore invocation counters shares mutable state so can't be run from many threads simultaneously
+public class TestHiveMetastoreJdbcMetadataQueriesAccessOperations


I would merge these tests classes together

agreed. there is little benefit in having AbstractTestHiveMetastoreMetadataQueriesAccessOperations
it can encourage us to have inconsistent (=> insufficient) test coverage between informaito_schema and system.jdbc

Merged together

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

findepi · 2023-04-28T12:52:25Z

@martint please merge

findepi · 2023-07-21T13:19:58Z

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

+        Multiset<Methods> invocations = metastore.getMethodInvocations();
+
+        assertThat(invocations.count(GET_TABLE)).as("GET_TABLE invocations")
+                // some lengthy explanatory comment why variable count


"some lengthy explanatory comment why variable count" from #17231 (comment) wasn't supposed to be treated literally, it was a placeholder :)

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

huberty89 requested review from kokosing and findepi April 25, 2023 15:17

huberty89 force-pushed the huberty89/fix-schema-prefixes-calculation branch from 84064b5 to 9f786ec Compare April 25, 2023 16:58

huberty89 commented Apr 25, 2023

View reviewed changes

...c/test/java/io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreReadAccessOperations.java Outdated Show resolved Hide resolved

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

huberty89 requested a review from Praveen2112 April 26, 2023 06:57

findepi reviewed Apr 26, 2023

View reviewed changes

kokosing reviewed Apr 26, 2023

View reviewed changes

Fix returned schema prefixes count

0fcea06

When there is more than 100 schemas InformationSchemaMetadata#calculatePrefixesWithSchemaName should return default prefix instead of all schemas

findepi reviewed Apr 26, 2023

View reviewed changes

...c/test/java/io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreReadAccessOperations.java Outdated Show resolved Hide resolved

...c/test/java/io/trino/plugin/hive/metastore/thrift/TestHiveMetastoreReadAccessOperations.java Outdated Show resolved Hide resolved

huberty89 force-pushed the huberty89/fix-schema-prefixes-calculation branch from 9f786ec to 7625801 Compare April 26, 2023 13:19

findepi added the RELEASE-BLOCKER label Apr 26, 2023

huberty89 force-pushed the huberty89/fix-schema-prefixes-calculation branch 7 times, most recently from 151ebf5 to 665b37c Compare April 27, 2023 05:50

Praveen2112 reviewed Apr 27, 2023

View reviewed changes

kokosing approved these changes Apr 27, 2023

View reviewed changes

huberty89 force-pushed the huberty89/fix-schema-prefixes-calculation branch from 665b37c to 9cf99b4 Compare April 27, 2023 10:50

findepi approved these changes Apr 27, 2023

View reviewed changes

Add tests to verify HMS request count for larger schema/table amount

33f8815

huberty89 force-pushed the huberty89/fix-schema-prefixes-calculation branch from 9cf99b4 to 33f8815 Compare April 28, 2023 09:02

findepi approved these changes Apr 28, 2023

View reviewed changes

findepi mentioned this pull request Apr 28, 2023

Fix enum name in CountingAccessHiveMetastore #17244

Merged

martint merged commit 14dde00 into trinodb:master Apr 28, 2023

github-actions bot added this to the 415 milestone Apr 28, 2023

huberty89 mentioned this pull request Apr 28, 2023

Fix returned schema prefixes count #17281

Closed

findepi reviewed Jul 21, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

huberty89 commented Apr 25, 2023

findepi Apr 26, 2023

huberty89 Apr 26, 2023

kokosing Apr 26, 2023

huberty89 Apr 26, 2023

findepi Apr 26, 2023

huberty89 Apr 26, 2023

findepi Apr 26, 2023

huberty89 Apr 26, 2023

findepi Apr 26, 2023

huberty89 Apr 26, 2023

huberty89 Apr 26, 2023

findepi Apr 26, 2023

huberty89 Apr 26, 2023

kokosing left a comment

kokosing Apr 26, 2023

kokosing Apr 26, 2023

huberty89 Apr 27, 2023

kokosing Apr 26, 2023

huberty89 Apr 27, 2023

kokosing Apr 27, 2023

findepi Apr 27, 2023

findepi Apr 27, 2023

findepi commented Apr 26, 2023 •

edited

Loading

Praveen2112 Apr 27, 2023

Praveen2112 Apr 27, 2023

huberty89 Apr 27, 2023

kokosing Apr 27, 2023

findepi Apr 27, 2023

huberty89 Apr 27, 2023

kokosing Apr 27, 2023

findepi Apr 27, 2023

huberty89 Apr 27, 2023

findepi commented Apr 28, 2023

findepi Jul 21, 2023

		methodInvocations.add(Methods.GET_ALL_TABLES_WITH_PARAMETER);
		return delegate.getAllTables(databaseName);

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

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

Conversation

huberty89 commented Apr 25, 2023

Description

Additional context and related issues

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

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

kokosing 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

findepi commented Apr 26, 2023 • edited Loading

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

findepi commented Apr 28, 2023

Choose a reason for hiding this comment

findepi commented Apr 26, 2023 •

edited

Loading