Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Apr 28, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fcea06
Fix returned schema prefixes count
Apr 25, 2023
33f8815
Add tests to verify HMS request count for larger schema/table amount
Apr 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions ...no-main/src/main/java/io/trino/connector/informationschema/InformationSchemaMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package io.trino.connector.informationschema;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -79,7 +80,8 @@ public class InformationSchemaMetadata
private static final InformationSchemaColumnHandle TABLE_NAME_COLUMN_HANDLE = new InformationSchemaColumnHandle("table_name");
private static final InformationSchemaColumnHandle ROLE_NAME_COLUMN_HANDLE = new InformationSchemaColumnHandle("role_name");
private static final InformationSchemaColumnHandle GRANTEE_COLUMN_HANDLE = new InformationSchemaColumnHandle("grantee");
private static final int MAX_PREFIXES_COUNT = 100;
@VisibleForTesting
public static final int MAX_PREFIXES_COUNT = 100;

private final String catalogName;
private final Metadata metadata;
Expand Down Expand Up @@ -247,9 +249,15 @@ private Set<QualifiedTablePrefix> calculatePrefixesWithSchemaName(
}

Session session = ((FullConnectorSession) connectorSession).getSession();
return listSchemaNames(session)
Set<QualifiedTablePrefix> schemaPrefixes = listSchemaNames(session)
.filter(prefix -> predicate.get().test(schemaAsFixedValues(prefix.getSchemaName().get())))
.collect(toImmutableSet());
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

testSelectTablesWithLikeOverSchema and testSelectTablesWithLikeOverTableName from added TestHiveMetastoreReadAccessOperations detect that case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed commit message with short explanation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was failing - exception from verify check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw thanks for fixing my bug

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to help

}
return schemaPrefixes;
}

private Set<QualifiedTablePrefix> calculatePrefixesWithTableName(
Expand Down
11 changes: 9 additions & 2 deletions .../trino-hive/src/test/java/io/trino/plugin/hive/metastore/CountingAccessHiveMetastore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@
import java.util.Set;
import java.util.function.Function;

import static io.trino.plugin.hive.metastore.CountingAccessHiveMetastore.Methods.GET_ALL_TABLES_FROM_DATABASE;
import static io.trino.plugin.hive.metastore.CountingAccessHiveMetastore.Methods.GET_ALL_VIEWS_FROM_DATABASE;

@ThreadSafe
public class CountingAccessHiveMetastore
implements HiveMetastore
Expand All @@ -44,8 +47,10 @@ public enum Methods
GET_ALL_DATABASES,
GET_DATABASE,
GET_TABLE,
GET_ALL_TABLES_FROM_DATABASE,
GET_TABLE_WITH_PARAMETER,
GET_TABLE_STATISTICS,
GET_ALL_VIEWS_FROM_DATABASE,
UPDATE_TABLE_STATISTICS,
ADD_PARTITIONS,
GET_PARTITION_NAMES_BY_FILTER,
Expand Down Expand Up @@ -113,7 +118,8 @@ public List<String> getTablesWithParameter(String databaseName, String parameter
@Override
public List<String> getAllViews(String databaseName)
{
throw new UnsupportedOperationException();
methodInvocations.add(GET_ALL_VIEWS_FROM_DATABASE);
return delegate.getAllViews(databaseName);
}

@Override
Expand Down Expand Up @@ -341,6 +347,7 @@ public void updatePartitionStatistics(Table table, Map<String, Function<Partitio
@Override
public List<String> getAllTables(String databaseName)
{
throw new UnsupportedOperationException();
methodInvocations.add(GET_ALL_TABLES_FROM_DATABASE);
return delegate.getAllTables(databaseName);
}
}
Loading