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.

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

Fix for SystemTables in allowSplittingReadIntoMultipleSubQueries #23529

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

Dith3r
Copy link
Member

@Dith3r Dith3r commented Sep 23, 2024

Description

Fix for cannot cast SystemTableHandle to ConnectorTableHandle in allowSplittingReadIntoMultipleSubQueries.

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

# Hive, Hudi, Iceberg, Delta
* Fix failures in queries with distinct aggregations on metadata tables. ({issue}`23529`)

@cla-bot cla-bot bot added the cla-signed label Sep 23, 2024
@github-actions github-actions bot added hudi Hudi connector iceberg Iceberg connector delta-lake Delta Lake connector hive Hive connector labels Sep 23, 2024
@raunaqmorarka raunaqmorarka added the bug Something isn't working label Sep 23, 2024
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.

Please add tests for this case.
Why do we get SystemTableHandle in this case and not all others where the table handle is cast directly to HiveTableHandle?

@@ -3984,7 +3984,12 @@ private static Optional<CatalogSchemaTableName> redirectTableToHudi(Optional<Str
@Override
public boolean allowSplittingReadIntoMultipleSubQueries(ConnectorSession session, ConnectorTableHandle tableHandle)
{
SchemaTableName tableName = ((HiveTableHandle) tableHandle).getSchemaTableName();
// dont split to subqueries if tableHandle is systemTableHandle
if (!(tableHandle instanceof HiveTableHandle hiveTableHandle)) {
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 check for the SystemTableHandle in the io.trino.metadata.MetadataManager#allowSplittingReadIntoMultipleSubQueries. This avoids copying this check to multiple places.

Copy link
Member

Choose a reason for hiding this comment

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

Filtering for SystemTableHandle in MetadataManager would indeed work for this specific case, but it's not clear to me that we won't encounter any more cases where tableHandle is different from HiveTableHandle.
It seems safer to just specifically test for connector specific table handle, even if there is code repetition.

Copy link
Member

Choose a reason for hiding this comment

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

You can't pass any table handle to any connector. Every connector expects only its own TableHandle class. The casts like that are all over the HiveMetadata. The CatalogHandle actually has io.trino.spi.connector.CatalogHandle#type that in this case is set to io.trino.spi.connector.CatalogHandle.CatalogHandleType#SYSTEM.

@Dith3r
Copy link
Member Author

Dith3r commented Sep 23, 2024

Why do we get SystemTableHandle in this case and not all others where the table handle is cast directly to HiveTableHandle?

Maybe there are cast issues which we are not aware of. Added test which without this fix fails.

@raunaqmorarka raunaqmorarka merged commit 2be471b into trinodb:master Sep 23, 2024
57 checks passed
@github-actions github-actions bot added this to the 459 milestone Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla-signed delta-lake Delta Lake connector hive Hive connector hudi Hudi connector iceberg Iceberg connector
Development

Successfully merging this pull request may close these issues.

3 participants