Skip to content

Conversation

@jayzzh
Copy link
Contributor

@jayzzh jayzzh commented Mar 8, 2024

Add partitions system table to Delta Lake Connector

This PR implements the $partitions system table in the Delta Lake Connector.

Additional context and related issues

Closes #18590. This PR iterates off a previously closed PR. I spoke with the original author and he is fine with me taking over where he has left off, so moving forward, I will add additional changes to closing out the issue.

In this PR, when querying $partitions, the returning columns are partition, file_count, and total_size, data.

cc @findinpath @ebyhr

Release notes

(x) Release notes are required, with the following suggested text:

# Delta Lake
* Add support for `$partitions` system table ({issue}`18590`)

@cla-bot
Copy link

cla-bot bot commented Mar 8, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@github-actions github-actions bot added docs delta-lake Delta Lake connector labels Mar 8, 2024
@cla-bot
Copy link

cla-bot bot commented Mar 8, 2024

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to [email protected]. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

@cla-bot cla-bot bot added the cla-signed label Mar 8, 2024
Copy link
Contributor

@findinpath findinpath left a comment

Choose a reason for hiding this comment

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

It seems to me that the PR is going in the right direction. Good job so far.

@findinpath
Copy link
Contributor

The failures in ci/pt (default, suire-delta-lake-oss) are related to your changes

| 2024-03-09 10:35:46 INFO: FAILURE     /    io.trino.tests.product.deltalake.TestHiveAndDeltaLakeRedirect.testHiveToPartitionedDeltaPartitionsRedirectFailure (Groups: profile_specific_tests, delta-lake-oss) took 1.5 seconds
tests               | 2024-03-09 10:35:46 SEVERE: Failure cause:
tests               | java.lang.AssertionError: Expected callback to throw QueryExecutionException
tests               | 	at io.trino.tempto.assertions.QueryAssert.assertQueryFailure(QueryAssert.java:113)
tests               | 	at io.trino.tests.product.deltalake.TestHiveAndDeltaLakeRedirect.testHiveToPartitionedDeltaPartitionsRedirectFailure(TestHiveAndDeltaLakeRedirect.java:154)

Copy link
Contributor

@findinpath findinpath left a comment

Choose a reason for hiding this comment

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

Functionality wise you're getting there.
Let's clarify the part related unpartitioned tables.

Thank you for your effort so far.

Copy link
Contributor

@findinpath findinpath left a comment

Choose a reason for hiding this comment

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

LGTM % last comment

@ebyhr ebyhr self-requested a review March 15, 2024 09:33
@findinpath
Copy link
Contributor

@jayzzh could you please continue the work on this PR?
We've done good progress so far. ❤️

@jayzzh jayzzh requested a review from ebyhr March 28, 2024 14:07
@jayzzh jayzzh requested a review from ebyhr March 29, 2024 14:02
@jayzzh jayzzh requested a review from ebyhr April 3, 2024 02:41
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Please fix CI failure.

Also, please add @whitleykeith as co-author of the commit.

@jayzzh jayzzh requested review from ebyhr and findinpath April 4, 2024 03:37
@jayzzh jayzzh requested a review from findinpath April 10, 2024 16:32
Copy link
Contributor

@findinpath findinpath left a comment

Choose a reason for hiding this comment

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

LGTM % comments

@jayzzh jayzzh requested a review from ebyhr April 11, 2024 13:21
@jayzzh jayzzh requested a review from ebyhr April 16, 2024 06:17
@jayzzh jayzzh requested a review from ebyhr April 17, 2024 03:49
@jayzzh jayzzh requested a review from ebyhr April 23, 2024 16:37
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Looks good except for data column implementation.

@jayzzh jayzzh requested a review from ebyhr April 26, 2024 01:09
@jayzzh jayzzh requested a review from ebyhr May 5, 2024 22:24
Copy link
Member

Choose a reason for hiding this comment

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

Please refactor the helper method to avoid passing redundant non-partition column a and b.

Copy link
Contributor Author

@jayzzh jayzzh May 8, 2024

Choose a reason for hiding this comment

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

Do you have recommendation on how not to pass in a and b? Also a and b are values in the partition column.

@ebyhr ebyhr merged commit dd2ddfb into trinodb:master May 20, 2024
@github-actions github-actions bot added this to the 449 milestone May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

Create $partitions system table for Delta Tables

5 participants