System tables optimization #18515

atanasenko · 2023-08-03T10:40:35Z

Description

This is a first draft pr which includes following optimizations for informations schema and system.jdbc tables:

Perform access control before filtering out data to prevent going over huge datasets
Use same approach for both info_schema and system.jdbc
Try to make better decisions before resorting to catalog-wide search

Additional context and related issues

I'm still reworking TestHiveMetastoreMetadataQueriesAccessOperations to validate more scenarios (access control, many tables-many filtered tables, many tables-few filtered tables, few tables in total, etc.)

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

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

atanasenko · 2023-08-03T10:42:09Z

core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java

@@ -757,18 +757,23 @@ protected Object visitArithmeticUnary(ArithmeticUnaryExpression node, Object con
                    if (handle.type().parameterCount() > 0 && handle.type().parameterType(0) == ConnectorSession.class) {


Please ignore this commit, I'll remove it later.
I'm using eclipse and ecj doesn't like yield within try for some reason. M2e also errors out on missing property.

kokosing

Just skimmed. Since it is optimization, do you have any numbers?

kokosing · 2023-08-03T12:12:16Z

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

@@ -119,25 +119,25 @@ public void testSelectTablesWithoutPredicate(boolean allTablesViewsImplemented)
        assertMetastoreInvocations("SELECT * FROM information_schema.tables",


Please extract this commit to separate PR so we can merge it fast

kokosing · 2023-08-03T12:13:09Z

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

@@ -39,6 +39,7 @@
 import io.trino.spi.connector.RowChangeParadigm;


Move table access control deeper into column listing

Can you explain in commit message why?

kokosing · 2023-08-03T12:14:03Z

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

@@ -171,7 +173,7 @@ Optional<TableExecuteHandle> getTableHandleForExecute(
     * Gets the columns metadata for all tables that match the specified prefix.
     * TODO: consider returning a stream for more efficient processing
     */
-    List<TableColumnsMetadata> listTableColumns(Session session, QualifiedTablePrefix prefix);
+    List<TableColumnsMetadata> listTableColumns(Session session, QualifiedTablePrefix prefix, UnaryOperator<Set<SchemaTableName>> tablesFilter);


UnaryOperator does not sound like filter. Did you mean Predicate?

Predicate just returns a boolean value, but we need to perform batch accessControl checks and return filtered entries.

kokosing · 2023-08-03T12:15:51Z

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

    {
        requireNonNull(prefix, "prefix is null");

        Optional<CatalogMetadata> catalog = getOptionalCatalogMetadata(session, prefix.getCatalogName());

        // Track column metadata for every object name to resolve ties between table and view
-        Map<SchemaTableName, Optional<List<ColumnMetadata>>> tableColumns = new HashMap<>();
+        List<Map<SchemaTableName, Optional<List<ColumnMetadata>>>> tableColumnMaps = new ArrayList<>();


Did you mean Multimap here? Also List sounds like an easy object to be merged with other List (maybe Set is even better here).

kokosing · 2023-08-03T12:18:13Z

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

@@ -630,9 +637,11 @@ public void testSelectColumnsWithLikeOverColumn(boolean allTablesViewsImplemente
    @Test
    public void testSelectColumnsFilterByTableAndSchema()
    {
-        assertMetastoreInvocations("SELECT * FROM information_schema.columns WHERE table_schema = 'test_schema_0' AND table_name = 'test_table_0'", ImmutableMultiset.of(GET_TABLE));


Please undo formatting changes

findepi · 2023-08-03T14:42:26Z

Perform access control before filtering out data to prevent going over huge datasets

That's a good point.
Please note however that listing tables is sometimes sufficient to get all information we need.
See this @ebyhr 's comment #18315 (review)
and see also #18517

I am not sure it's obvious which direction to go into.

findepi · 2023-08-04T07:22:47Z

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

-                        .addCopies(GET_TABLE, TEST_ALL_TABLES_COUNT)
-                        .build()
+                                .add(GET_ALL_DATABASES)
+                                .add(GET_ALL_TABLES)


We need to agree on intellij configuration.
The current ("before") code is like my intellij happens to format the code.
The new ("after") code at least my intellij would not accept, and would revert back to the indentation "before"

I believe my formatter was taken from the same repo. Let me update it.

I noticed that even without formatter definition changes, different intellij versions may render the code slightly differently. For pragmatic reasons we should for now simply standardize on eg latest stable ide version.

for this PR, feel free to simply undo formatting changes.

findepi · 2023-08-04T07:23:53Z

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

+        final long schemaPrefixLimit = Math.min(maxPrefetchedInformationSchemaPrefixes, schemaPrefixes.size());
+        final long tablePrefixLimit = Math.max(maxPrefetchedInformationSchemaPrefixes, schemaPrefixLimit * TABLE_COUNT_PER_SCHEMA_THRESHOLD);


style: we don't use final for variables

findepi · 2023-08-04T07:26:12Z

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

+    @VisibleForTesting
+    public static final int TABLE_COUNT_PER_SCHEMA_THRESHOLD = 3;


@VisibleForTesting is usually on package-private stuff

what is semantics of TABLE_COUNT_PER_SCHEMA_THRESHOLD constant? please define

More importantly, i am concerned about "Extract information schema data filtering into a separate class" not being pure refactor, despite the name suggesting so. At least, I couldn't find any "threshold" constant in the code being (re)moved.

I think one of the other commits got accidentally merged after a series of interactive rebases. It was supposed to be a pure refactor.

findepi · 2023-08-04T07:26:24Z

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

+import static java.util.Locale.ENGLISH;
+import static java.util.Objects.requireNonNull;
+
+public final class SystemTableFilter<T>


can this new class have a javadoc hinting what is it for?

findepi · 2023-08-04T13:28:26Z

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

+     * Gets the metadata for all columns that match the specified table prefix. Redirected table names are included, but the column metadata for them is not. Additional
+     * <code>tableFilter</code> parameter can be used to prune tables from column search.
+     */
+    default Iterator<TableColumnsMetadata> streamTableColumns(ConnectorSession session, SchemaTablePrefix prefix, UnaryOperator<Set<SchemaTableName>> tablesFilter)


There already is a method with this name in this interface (streamTableColumns(ConnectorSession session, SchemaTablePrefix prefix))

delegate to the existing method

deprecate the old method, since we want to eventually remove it

moreover, following applyFilter as example, we could consider not requiring the connector to apply tablesFilter and treat this as advisory. If we do so, we would need the connector to be able to tell whether it applied the filter. We should not apply the filter twice, as it may be expensive (depending on number of tables). Not sure about this, but this at least is the direction i took in #18517.

moreover, following applyFilter as example, we could consider not requiring the connector to apply tablesFilter and treat this as advisory

i think it's simpler if we require implementations to apply the filter.
shouldn't be a big deal and simplifies the API. i reworked #18517 following this idea.

Going further, we could allow the connector to fetch column information for MVs, views and tables in one call. This would let avoid listing relations 3 times, when compared to getMaterializedViews + getViews + streamTableColumns currently used to satisfy information_schema.columns.
I drafted a PR showing how this could look like: #18585

findepi · 2023-08-04T13:28:58Z

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

+                catch (TrinoException e) {
+                    if (e.getErrorCode().equals(NOT_SUPPORTED.toErrorCode())) {
+                        columnsIterator = metadata.streamTableColumns(connectorSession, tablePrefix);
+                        needsExplicitFiltering = true;


if new streamTableColumns delegates to the old streamTableColumns, then the logic here doesn't need to be exception driven

findepi · 2023-08-04T13:30:47Z

core/trino-main/src/main/java/io/trino/connector/system/jdbc/ColumnJdbcTable.java

@@ -19,12 +19,11 @@
 import io.airlift.slice.Slices;
 import io.trino.FullConnectorSession;
 import io.trino.Session;
+import io.trino.connector.informationschema.SystemTableFilter;


Change schema/table prefix behavior in system.jdbc and info_schema

Assuming "info_schema" is an abbreviation for information_schema, please spell it out.

Also, the commit seems not to touch information_schema.
So spell out "system.jdbc.columns"

Schema and table prefixes are treated equal when limiting their numbers
during column queries in system.jdbc and information_schema. But in
reality fetching tables from 100 schemas and then filtering them in
memory is way more expensive than fetching 100 tables directly.

This is especially true if schemas have lots of tables underneath that
we will have to go through.

Thanks for adding some color to the commit message. This introduces the problem it is attempting to solve. Can the commit message also summarize the solution?

findepi · 2023-08-04T13:34:31Z

core/trino-main/src/test/java/io/trino/metadata/TestInformationSchemaMetadata.java

@@ -118,7 +121,7 @@ public void testInformationSchemaPredicatePushdown()
        Constraint constraint = new Constraint(TupleDomain.withColumnDomains(domains.buildOrThrow()));

        ConnectorSession session = createNewSession(transactionId);
-        ConnectorMetadata metadata = new InformationSchemaMetadata("test_catalog", this.metadata, MAX_PREFIXES_COUNT);
+        ConnectorMetadata metadata = new InformationSchemaMetadata("test_catalog", this.metadata, this.accessControl, MAX_PREFIXES_COUNT);


is this. redundant?

atanasenko · 2023-08-09T11:24:35Z

Commit with streamTableColumns filtering will be superseded by #18586

Schema and table prefixes are treated equal when limiting their numbers during column queries in system.jdbc and information_schema. But in reality fetching tables from 100 schemas and then filtering them in memory is way more expensive than fetching 100 tables directly. This is especially true if schemas have lots of tables underneath that we will have to go through.

findepi · 2023-08-09T11:36:25Z

Commit with streamTableColumns filtering will be superseded by #18586

Thanks for the info.
You may also rebase onto it, if you want.
Move table access control deeper into column listing will probably get obsoleted too.

BTW i see there are unapplied/unanswered comments (e.g. #18515 (comment)), which is understood as WIP.
Let me know when I should review again.

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

atanasenko commented Aug 3, 2023

View reviewed changes

atanasenko requested review from findepi, kokosing and Praveen2112 August 3, 2023 10:42

atanasenko force-pushed the at/system-tables-optimization branch from 8057b4f to f6d3d47 Compare August 3, 2023 10:50

github-actions bot added tests:hive hive Hive connector labels Aug 3, 2023

kokosing reviewed Aug 3, 2023

View reviewed changes

findepi mentioned this pull request Aug 3, 2023

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

Merged

findepi reviewed Aug 4, 2023

View reviewed changes

atanasenko force-pushed the at/system-tables-optimization branch from f6d3d47 to b431c66 Compare August 9, 2023 11:22

atanasenko added 6 commits August 9, 2023 14:25

tmpdev

34e0dea

Move table access control deeper into column listing

f290fea

Extract information schema data filtering into a separate class

23643db

Add access control checks to SystemTableFilter

a2d6a4e

Adjust test class formatting

50487a0

atanasenko force-pushed the at/system-tables-optimization branch from b431c66 to 0ba9d57 Compare August 9, 2023 11:32

github-actions bot added the jdbc Relates to Trino JDBC driver label Aug 9, 2023

findepi removed the tests:hive label Apr 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

System tables optimization #18515

System tables optimization #18515

atanasenko commented Aug 3, 2023 •

edited

Loading

atanasenko Aug 3, 2023

kokosing left a comment

kokosing Aug 3, 2023

kokosing Aug 3, 2023

kokosing Aug 3, 2023

atanasenko Aug 3, 2023

kokosing Aug 3, 2023

kokosing Aug 3, 2023

findepi commented Aug 3, 2023

findepi Aug 4, 2023

atanasenko Aug 8, 2023

findepi Aug 9, 2023

findepi Aug 4, 2023

findepi Aug 4, 2023

atanasenko Aug 8, 2023

findepi Aug 4, 2023

findepi Aug 4, 2023

findepi Aug 4, 2023

findepi Aug 8, 2023

findepi Aug 4, 2023

findepi Aug 4, 2023

findepi Aug 4, 2023

findepi Aug 4, 2023

atanasenko commented Aug 9, 2023

findepi commented Aug 9, 2023

		@@ -757,18 +757,23 @@ protected Object visitArithmeticUnary(ArithmeticUnaryExpression node, Object con
		if (handle.type().parameterCount() > 0 && handle.type().parameterType(0) == ConnectorSession.class) {

		@@ -119,25 +119,25 @@ public void testSelectTablesWithoutPredicate(boolean allTablesViewsImplemented)
		assertMetastoreInvocations("SELECT * FROM information_schema.tables",

		@@ -39,6 +39,7 @@
		import io.trino.spi.connector.RowChangeParadigm;

		final long schemaPrefixLimit = Math.min(maxPrefetchedInformationSchemaPrefixes, schemaPrefixes.size());
		final long tablePrefixLimit = Math.max(maxPrefetchedInformationSchemaPrefixes, schemaPrefixLimit * TABLE_COUNT_PER_SCHEMA_THRESHOLD);

		@VisibleForTesting
		public static final int TABLE_COUNT_PER_SCHEMA_THRESHOLD = 3;

System tables optimization #18515

Are you sure you want to change the base?

System tables optimization #18515

Conversation

atanasenko commented Aug 3, 2023 • edited Loading

Description

Additional context and related issues

Release notes

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

findepi commented Aug 3, 2023

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

Choose a reason for hiding this comment

atanasenko commented Aug 9, 2023

findepi commented Aug 9, 2023

atanasenko commented Aug 3, 2023 •

edited

Loading