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

Jump to bottom

[WIP] Improve performance of querying system.jdbc.tables for Hive, Iceberg, and Delta #24110

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

[WIP] Improve performance of querying system.jdbc.tables for Hive, Iceberg, and Delta #24110

wants to merge 4 commits into from
+353 −63

Conversation

piotrrzysko
Copy link
Member

@piotrrzysko piotrrzysko commented Nov 12, 2024 •
edited
Loading

Description

For now, we are mainly focused on improving performance for connectors using Glue.

Locally, the execution time of the following query, for 6500 tables and 530 schemas, decreased from 3m 11s to 27s:

SELECT TABLE_CAT, TABLE_SCHEM, TABLE_NAME, TABLE_TYPE, REMARKS,
  TYPE_CAT, TYPE_SCHEM, TYPE_NAME,   SELF_REFERENCING_COL_NAME, REF_GENERATION
FROM system.jdbc.tables
WHERE TABLE_CAT = 'hive' AND TABLE_TYPE IN ('TABLE', 'VIEW')
ORDER BY TABLE_TYPE, TABLE_CAT, TABLE_SCHEM, TABLE_NAME;

I'm marking this as a draft because there are a few questions included in commit messages that need to be addressed.

Additional context and related issues

Release notes

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

@cla-bot cla-bot bot added the cla-signed label Nov 12, 2024
@github-actions github-actions bot added iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector labels Nov 12, 2024
@piotrrzysko piotrrzysko changed the title [WIP] Improve performance of querying system.jdbc.tables for Hive, Iceberg, and Delta [WIP] Improve performance of querying system.jdbc.tables for Hive, Iceberg, and Delta Nov 12, 2024
@shohamyamin
Copy link

Hi there!

I came across this PR and was curious if it might address a similar issue to the one in my PR #23909, which focuses on improving the retrieval of column metadata for Iceberg tables. In my PR, I added parallelization to the streamTableColumns method in IcebergMetadata.java, enhancing performance for users pulling column metadata.

I noticed there haven’t been any changes related to the REST catalog in this PR. Would you consider applying similar improvements to the REST catalog as well? This would allow users working with REST catalog setups to benefit from the enhanced metadata retrieval. Also, could you provide some details about the use cases this PR aims to support?

Thanks!

@piotrrzysko
Parallelize Hive tables retrieval
9388bf2 
The parameter for specifying the maximum number of threads fetching
tables ("hive.metadata.parallelism") aligns with the naming convention
used in the BigQuery connector ("bigquery.metadata.parallelism").

Parallelization has been introduced in HiveMetadata rather than in
specific metastore implementations, primarily to avoid reintroducing a
cache storing tables for all schemas, which was removed in
trinodb@cb4d168.
This approach attempts to parallelize table retrieval for all metastore
types, even though not all support concurrent access. Currently, only
the FileHiveMetastore does not support multithreaded access, making
parallelization ineffective.

Question: Should we consider setting the default value of
"hive.metadata.parallelism" to 1 when using the "file" metastore?
@piotrrzysko
Parallelize Delta Lake tables retrieval
48633f5 
Before introducing DeltaLakeMetadata::getRelationTypes,
ConnectorMetadata::getRelationTypes was used to retrieve relation types
for Delta Lake. The original implementation classified all tables as
RelationType.TABLE, except those with the extended relational type
TRINO_VIEW, which were classified as RelationType.VIEW. This is why the
resolveRelationType method was added in this commit.

Question: Is this resolution necessary? Could we instead use the
existing mapping between ExtendedRelationType and RelationType that's
already encapsulated in RelationType?
@piotrrzysko
Parallelize Iceberg tables retrieval
f7f8579 
Parallelization has been implemented at the TrinoCatalog level, rather
than in IcebergMetadata, because some catalogs (e.g., Nessie) seem to
support optimized table retrieval across all schemas. Currently,
parallelization has been added for Glue and Hive catalogs, but it can
easily be extended to other catalogs as well.
@piotrrzysko
Copy link
Member Author

@shohamyamin

I came across this PR and was curious if it might address a similar issue to the one in my #23909, which focuses on improving the retrieval of column metadata for Iceberg tables.

This PR focuses on improving the performance of table listing. It won't address column retrieval, but it should be relatively straightforward to extend the current changes to include columns as well.

I noticed there haven’t been any changes related to the REST catalog in this PR. Would you consider applying similar improvements to the REST catalog as well?

Currently, I'm mainly focused on improving performance for the Glue catalog, but the same approach can be applied to other catalogs as well.

Also, could you provide some details about the use cases this PR aims to support?

The goal is to improve the performance of queries like the one in the PR's description, which are often issued by DB tools to list tables.

@hashhar hashhar self-requested a review November 14, 2024 14:58
lukasz-stec
lukasz-stec approved these changes Nov 15, 2024
View reviewed changes
Copy link
Member

@lukasz-stec lukasz-stec left a comment

Choose a reason for hiding this comment

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

LGTM, that will be a great improvement for glue catalogs with many schemas!.
Please provide benchmark results once you have it, both in PR description but also in the commit messages if applicable.

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java
}
}

private RelationType resolveRelationType(TableInfo tableInfo)
Copy link
Member

Choose a reason for hiding this comment

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

@piotrrzysko I would drop the question from the commit message.
The question was:

Is this resolution necessary? Could we instead use the
existing mapping between ExtendedRelationType and RelationType that's
already encapsulated in RelationType?

I guess the question here is, is OTHER_VIEW or OTHER_MATERIALIZED_VIEW possible in delta-lake? TRINO_MATERIALIZED_VIEW is not.
@raunaqmorarka @dain Do you know?

Copy link
Member

Choose a reason for hiding this comment

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

btw it is ok for me to have the resolveRelationType here to keep the existing functionality

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeConfig.java
@@ -90,6 +90,7 @@ public class DeltaLakeConfig
private boolean queryPartitionFilterRequired;
private boolean deletionVectorsEnabled;
private boolean deltaLogFileSystemCacheDisabled;
private int metadataParallelism = 8;
Copy link
Member

Choose a reason for hiding this comment

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

8 seems pretty low. Did you test bigger number of threads?

plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveConfig.java
@@ -176,6 +176,8 @@ public class HiveConfig

private S3StorageClassFilter s3StorageClassFilter = S3StorageClassFilter.READ_ALL;

private int metadataParallelism = 8;
Copy link
Member

Choose a reason for hiding this comment

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

I would drop the question from the commit message.
The question is

Question: Should we consider setting the default value of
"hive.metadata.parallelism" to 1 when using the "file" metastore?

I would not worry about file metastore. @raunaqmorarka WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raunaqmorarka raunaqmorarka raunaqmorarka left review comments

@lukasz-stec lukasz-stec lukasz-stec approved these changes

@hashhar hashhar Awaiting requested review from hashhar

Assignees
No one assigned
Labels
cla-signed delta-lake Delta Lake connector hive Hive connector iceberg Iceberg connector
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@piotrrzysko @shohamyamin @raunaqmorarka @lukasz-stec