Add `$partitions` table to Delta Connector #19722

whitleykeith · 2023-11-13T17:12:24Z

Description

This PR implements the $partitions system table for the Delta Lake connector, and behaves similarly to the Hive analog. It additionally adds a .devcontainer.json to support running Trino in a devcontainer/codespace. I did this primarily for my own workflow and can remove it if it's not desired.

An implementation wrinkle: Due to how the AddFileEntry.getCanonicalPartitionValues returns strings for partition values and some issues with casting back to their original types, the columns in the result table are always VARCHAR, even if the underlying column is a different type. I called this out in the related doc updates.

Additional context and related issues

Closes #18590

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:

# Delta Lake
* Add `$partitions` system table to the Delta Connector ({issue}`18590`)

cla-bot · 2023-11-13T17:12:28Z

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 · 2023-11-13T17:12:58Z

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

findinpath · 2023-11-13T17:34:19Z

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

+        this.partitionColumns = requireNonNull(partitionColumns, "partitionColumns is null");
+
+        ImmutableList.Builder<ColumnMetadata> columns = ImmutableList.builder();
+        partitionColumns.forEach(column -> columns.add(new ColumnMetadata(column.getBaseColumnName(), VARCHAR)));


i don't follow the reasoning behind this constraint - why not using the actual type of the partition column?

I initially tried to do that, but there were issues with casting back to the original type as AddFileEntry.getCanonicalPartitionValues() returns a string represenation of the value

It should be the original type. Also, we need to discuss the result format (Hive or Iceberg)

-- Hive select * from "test$partitions"; b --- 1 -- Iceberg select * from "test$partitions"; partition | record_count | file_count | total_size | data -----------+--------------+------------+------------+-------------------------------------------------- {b=1} | 1 | 1 | 317 | {a={min=1, max=1, null_count=0, nan_count=NULL}}

I'll try to see if I can get the types correct 👍

Good point on format, I've only used the hive and delta connectors in practice so went with the original Hive format. The Iceberg format has a lot more info, but I'm not sure how best to materialize that from the transaction log. I'll defer to one of you on which format we want

Thanks for the review as well, I'll take a look at the other comments and fix this PR up. Appreciate the patience for a first time contributor 🙏

This commit adds the `DeltaLakePartitionsTable` and `DeltaLakePartitionsTableProvider` classes to support the `$partitions` system table in the Delta Connector. As the partition values casted as string from internal calls, the resulting table will always return VARCHAR, so called that out in the docs.

cla-bot · 2023-11-13T17:47:25Z

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 · 2023-11-13T17:47:55Z

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

ebyhr · 2023-11-13T22:13:36Z

.devcontainer/devcontainer.json

@@ -0,0 +1,18 @@
+{


Remove an unrelated change.

ebyhr · 2023-11-13T22:13:54Z

docs/src/main/sphinx/connector/delta-lake.md

@@ -704,6 +704,49 @@ delta.feature.columnMapping | supported       |

 (delta-lake-special-columns)=

+#### `$partitions` table
+
+The `$partitions` table provides access to a specified Delta Lake Table's partitions keys and values. Each table row is a unique partition value, with the columns being the partition keys. 


The line is too long. Please wrap at 80 characters.

ebyhr · 2023-11-13T22:14:45Z

docs/src/main/sphinx/connector/delta-lake.md

+ name                       | comment         | regionkey           |
+----------------------------+-----------------+---------------------+
+foo                         | foo             | east                |
+bar                         | bar             | west                |
+baz                         | baz             | west                |


This table looks too wide. Please narrow it. Same for the following $partitions result.

ebyhr · 2023-11-13T22:57:08Z

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

    }

-    private Optional<SystemTable> getRawSystemTable(SchemaTableName systemTableName)
+    private Optional<SystemTable> getRawSystemTable(SchemaTableName systemTableName, ConnectorSession session)


Replace the order between SchemaTableName & ConnectorSession.

ebyhr · 2023-11-13T23:06:11Z

plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeSystemTables.java

+            assertUpdate("INSERT INTO " + tableName + " VALUES (0, CAST('2019-09-08' AS DATE)), (1, CAST('2019-09-09' AS DATE)), (2, CAST('2019-09-09' AS DATE))", 3);
+            assertUpdate("INSERT INTO " + tableName + " VALUES (3, CAST('2019-09-09' AS DATE)), (4, CAST('2019-09-10' AS DATE)), (5, CAST('2019-09-10' AS DATE))", 3);


Please test with NULL partitions.

ebyhr · 2023-11-13T23:30:21Z

.devcontainer/devcontainer.json

@@ -0,0 +1,18 @@
+{


Please rebase on master instead of merge and then fix logical conflicts.

Error: Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project trino-delta-lake: Compilation failure: Compilation failure: Error: /home/runner/work/trino/trino/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakePartitionsTable.java:[95,49] cannot find symbol Error: symbol: method getSnapshot(io.trino.spi.connector.ConnectorSession,io.trino.spi.connector.SchemaTableName,java.lang.String,java.util.Optional<java.lang.Object>) Error: location: variable transactionLogAccess of type io.trino.plugin.deltalake.transactionlog.TransactionLogAccess Error: /home/runner/work/trino/trino/plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakePartitionsTableProvider.java:[57,63] cannot find symbol Error: symbol: method getSnapshot(io.trino.spi.connector.ConnectorSession,io.trino.spi.connector.SchemaTableName,java.lang.String,java.util.Optional<java.lang.Object>) Error: location: variable transactionLogAccess of type io.trino.plugin.deltalake.transactionlog.TransactionLogAccess Error: -> [Help 1]

ebyhr · 2023-11-13T23:34:13Z

plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeSystemTables.java

+    @Test
+    public void testPartitionsTable()
+    {
+        String tableName = "test_simple_partitions_table";


All 3 tests have _simple_ in the table name. It doesn't provide any benefits unless we have complex tests. I would remove them.

ebyhr · 2023-11-13T23:40:25Z

plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeSystemTables.java

+            assertUpdate("INSERT INTO " + tableName + " VALUES (3, CAST('2019-09-09' AS DATE)), (4, CAST('2019-09-10' AS DATE)), (5, CAST('2019-09-10' AS DATE))", 3);
+            assertUpdate("UPDATE " + tableName + " SET _bigint = 50 WHERE _bigint =  BIGINT '5'", 1);
+            assertUpdate("DELETE FROM " + tableName + " WHERE _date =  DATE '2019-09-08'", 1);
+            assertQuerySucceeds("ALTER TABLE " + tableName + " EXECUTE OPTIMIZE");
+            assertQuery("SELECT count(*) FROM " + tableName, "VALUES 5");
+            assertQuery("SELECT count(*) FROM \"" + tableName + "$partitions\"", "VALUES 0");


I would remove these statements. Single INSERT is sufficient for verifying $partitons with unpartitioned table.

ebyhr · 2023-11-13T23:46:25Z

docs/src/main/sphinx/connector/delta-lake.md

@@ -704,6 +704,49 @@ delta.feature.columnMapping | supported       |

 (delta-lake-special-columns)=

+#### `$partitions` table


The current place (after (delta-lake-special-columns)=) is wrong.

ebyhr · 2023-11-13T23:46:43Z

docs/src/main/sphinx/connector/delta-lake.md

+
+

Remove a redundant empty line.

github-actions · 2024-01-10T17:23:05Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-01-11T19:54:47Z

👋 @whitleykeith - this PR has become inactive. If you're still interested in working on it, please let us know, and we can try to get reviewers to help with that.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

github-actions · 2024-02-02T17:03:55Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-02-02T20:52:51Z

@findinpath @ebyhr .. is this still useful/valid or should we close?

whitleykeith · 2024-02-14T23:15:08Z

@mosabua I'm the original author of the PR. Sorry for not chiming in here, I chatted with @findinpath last month in slack about this. We use Trino a lot, especially the Delta connector, and I would love to be able to get this in a good state. I ran out of time when I first started development on this but I'm hoping I can block off some time in the next few weeks to resolve comments + make necessary changes

mosabua · 2024-02-14T23:43:51Z

Thats great news @whitleykeith .. feel free to continue to work with @findinpath and others and let me know if you need any other help.

whitleykeith · 2024-02-16T16:00:39Z

It was easier to make a brand new PR since there was a lot of changes (including a java version upgrade), new PR is here: #20735

github-actions bot added docs delta-lake Delta Lake connector labels Nov 13, 2023

findinpath self-requested a review November 13, 2023 17:26

findinpath reviewed Nov 13, 2023

View reviewed changes

whitleykeith force-pushed the whitleykeith/delta-partitions-table branch from 601089c to 66626ae Compare November 13, 2023 17:47

Merge branch 'master' into whitleykeith/delta-partitions-table

9c19f29

ebyhr reviewed Nov 13, 2023

View reviewed changes

github-actions bot added the stale label Jan 10, 2024

github-actions bot removed the stale label Jan 12, 2024

github-actions bot added the stale label Feb 2, 2024

github-actions bot removed the stale label Feb 5, 2024

whitleykeith mentioned this pull request Feb 16, 2024

Add $partitions system table to Delta Lake Connector #20735

Closed

whitleykeith closed this Feb 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `$partitions` table to Delta Connector #19722

Add `$partitions` table to Delta Connector #19722

whitleykeith commented Nov 13, 2023 •

edited

Loading

cla-bot bot commented Nov 13, 2023

cla-bot bot commented Nov 13, 2023

findinpath Nov 13, 2023

whitleykeith Nov 13, 2023

ebyhr Nov 13, 2023 •

edited

Loading

whitleykeith Nov 14, 2023 •

edited

Loading

cla-bot bot commented Nov 13, 2023

cla-bot bot commented Nov 13, 2023

ebyhr Nov 13, 2023

ebyhr Nov 13, 2023

ebyhr Nov 13, 2023 •

edited

Loading

ebyhr Nov 13, 2023

ebyhr Nov 13, 2023

ebyhr Nov 13, 2023 •

edited

Loading

ebyhr Nov 13, 2023

ebyhr Nov 13, 2023

ebyhr Nov 13, 2023

ebyhr Nov 13, 2023

github-actions bot commented Jan 10, 2024

mosabua commented Jan 11, 2024

github-actions bot commented Feb 2, 2024

mosabua commented Feb 2, 2024

whitleykeith commented Feb 14, 2024

mosabua commented Feb 14, 2024

whitleykeith commented Feb 16, 2024

		assertUpdate("INSERT INTO " + tableName + " VALUES (0, CAST('2019-09-08' AS DATE)), (1, CAST('2019-09-09' AS DATE)), (2, CAST('2019-09-09' AS DATE))", 3);
		assertUpdate("INSERT INTO " + tableName + " VALUES (3, CAST('2019-09-09' AS DATE)), (4, CAST('2019-09-10' AS DATE)), (5, CAST('2019-09-10' AS DATE))", 3);

		@@ -704,6 +704,49 @@ delta.feature.columnMapping \| supported \|

		(delta-lake-special-columns)=

		#### `$partitions` table

Add $partitions table to Delta Connector #19722

Add $partitions table to Delta Connector #19722

Conversation

whitleykeith commented Nov 13, 2023 • edited Loading

Description

Additional context and related issues

Release notes

cla-bot bot commented Nov 13, 2023

cla-bot bot commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

whitleykeith Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

cla-bot bot commented Nov 13, 2023

cla-bot bot commented Nov 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Nov 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebyhr Nov 13, 2023 • edited Loading

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

github-actions bot commented Jan 10, 2024

mosabua commented Jan 11, 2024

github-actions bot commented Feb 2, 2024

mosabua commented Feb 2, 2024

whitleykeith commented Feb 14, 2024

mosabua commented Feb 14, 2024

whitleykeith commented Feb 16, 2024

Add `$partitions` table to Delta Connector #19722

Add `$partitions` table to Delta Connector #19722

whitleykeith commented Nov 13, 2023 •

edited

Loading

ebyhr Nov 13, 2023 •

edited

Loading

whitleykeith Nov 14, 2023 •

edited

Loading

ebyhr Nov 13, 2023 •

edited

Loading

ebyhr Nov 13, 2023 •

edited

Loading