Partition Column transform support for Year, Month, Day & Hour by gupteaj · Pull Request #21303 · prestodb/presto

gupteaj · 2023-11-03T17:58:35Z

Description

Presto issue #20570

Fix getColumnTransform() for date and timestamp columns to support transform based on iceberg partition spec
Add a new function transformBlock() to write partition column values using INTEGER type

Motivation and Context

Beyond selecting a particular column to partition by, you can select a “transform” and partition the table by the transformed value of the column.

Available in Iceberg include: https://iceberg.apache.org/spec/#partition-specs
Day, Month, Year, Hour, Bucket, Truncate

Presto Iceberg Connector currently already supports Bucket & Truncate transform with partition column in Iceberg Table. This feature request is to add transform support for Day, Month, Year, Hour

Impact

Presto Iceberg connector will support these partition transform functions.

Test Plan

Add new cases for Year, Month, Day, Hour in IcebergDistributedSmokeTestBase.java
Manual testing

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==


Iceberg Changes
* Day, Month & Year partition column transform functions are supported for date type in Presto Iceberg connector.
* Day, Month, Year & Hour partition column transform functions are supported for timestamp type in Presto Iceberg connector.

steveburnett

Thanks for the documentation! I made suggestions for consistency with examples earlier in the page, and a very small formatting error.

If my rephrasing suggestions change your intended meaning in a way you disagree with, let me know and I'm happy to discuss it.

presto-docs/src/main/sphinx/connector/iceberg.rst

steveburnett

My apologies! I overlooked this single edit but I focused on the code block edit. I should have included this suggested change in yesterday's review. This is everything, no other suggestions.

presto-docs/src/main/sphinx/connector/iceberg.rst

steveburnett

LGTM! (docs)

Thanks!

github-actions · 2023-11-16T18:24:43Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 038ff56...b7c6bff.

Notify	File(s)
@steveburnett	presto-docs/src/main/sphinx/connector/iceberg.rst

steveburnett

(redoing review because CODENOTIFY pinged me)

LGTM! (docs)

New local build of docs, everything looks great.

presto-iceberg/src/main/java/com/facebook/presto/iceberg/PartitionTransforms.java

presto-docs/src/main/sphinx/connector/iceberg.rst

steveburnett

Nice work! Two very small nits to consider.

presto-docs/src/main/sphinx/connector/iceberg.rst

hantangwangd

There are some problems when calculating partition values.

presto-iceberg/src/main/java/com/facebook/presto/iceberg/PartitionTransforms.java

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

steveburnett

LGTM! (docs)

Thanks!

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

…r() functions.

hantangwangd

LGTM! Thanks for the fix.

tdcmeehan · 2023-12-19T16:15:07Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

+        assertUpdate("INSERT INTO " + tableName + " VALUES (3, date '1970-10-01'), (4, date '1971-11-05')", 2);
+
+        assertQuery("SELECT c2_day, row_count, file_count FROM " + "\"" + tableName + "$partitions\" ORDER BY c2_day",
+                "VALUES ('1970-10-01', 1, 1), ('1971-11-05', 1, 1), ('2022-10-01', 1, 1), ('2023-11-02', 1, 1)");


I am a bit confused. According to the spec, partition transform by day should return an integer. Shouldn't c2_day then return the number of days from 1970-01-01, not the string date?

It seems that Iceberg implementation has special treatment for partition function day. Referring to the codes in function org.apache.iceberg.transforms.Dates.getResultType().

You are correct. I raised apache/iceberg#9345 to fix this.

gupteaj requested review from s-akhtar-baig, steveburnett and tdcmeehan November 3, 2023 17:58

gupteaj mentioned this pull request Nov 3, 2023

Partition Column transform support in Iceberg Connector #20570

Closed

steveburnett requested changes Nov 6, 2023

View reviewed changes

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

gupteaj force-pushed the test_transform branch from 90b4690 to 29c39ca Compare November 6, 2023 22:55

gupteaj requested a review from steveburnett November 6, 2023 22:56

steveburnett requested changes Nov 7, 2023

View reviewed changes

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

gupteaj force-pushed the test_transform branch from 29c39ca to a19eff2 Compare November 7, 2023 18:30

gupteaj requested a review from steveburnett November 7, 2023 18:31

tdcmeehan self-assigned this Nov 7, 2023

steveburnett approved these changes Nov 7, 2023

View reviewed changes

gupteaj force-pushed the test_transform branch 3 times, most recently from 6da5a77 to 0e3fde1 Compare November 15, 2023 06:49

gupteaj marked this pull request as ready for review November 16, 2023 18:23

gupteaj requested a review from a team as a code owner November 16, 2023 18:23

gupteaj requested review from ChunxuTang, hantangwangd and presto-oss November 16, 2023 18:23

steveburnett approved these changes Nov 16, 2023

View reviewed changes

tdcmeehan reviewed Nov 16, 2023

View reviewed changes

presto-iceberg/src/main/java/com/facebook/presto/iceberg/PartitionTransforms.java Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

gupteaj force-pushed the test_transform branch from 0e3fde1 to ef75f7b Compare November 16, 2023 22:33

gupteaj requested review from steveburnett and tdcmeehan November 16, 2023 22:35

steveburnett requested changes Nov 17, 2023

View reviewed changes

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

tdcmeehan approved these changes Nov 17, 2023

View reviewed changes

gupteaj force-pushed the test_transform branch from ef75f7b to 93f2094 Compare November 17, 2023 19:55

gupteaj requested a review from steveburnett November 17, 2023 19:56

gupteaj force-pushed the test_transform branch from 93f2094 to 012549c Compare November 17, 2023 23:08

hantangwangd reviewed Nov 18, 2023

View reviewed changes

presto-iceberg/src/main/java/com/facebook/presto/iceberg/PartitionTransforms.java Outdated Show resolved Hide resolved

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java Outdated Show resolved Hide resolved

gupteaj marked this pull request as draft November 19, 2023 06:34

steveburnett approved these changes Nov 20, 2023

View reviewed changes

kiersten-stokes reviewed Nov 21, 2023

View reviewed changes

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java Outdated Show resolved Hide resolved

Partition Column transform support for year(), month(), day() and hou…

b7c6bff

…r() functions.

gupteaj force-pushed the test_transform branch from 012549c to b7c6bff Compare December 19, 2023 06:03

gupteaj marked this pull request as ready for review December 19, 2023 06:08

gupteaj requested review from hantangwangd, kiersten-stokes and tdcmeehan December 19, 2023 06:08

hantangwangd approved these changes Dec 19, 2023

View reviewed changes

tdcmeehan reviewed Dec 19, 2023

View reviewed changes

tdcmeehan approved these changes Dec 19, 2023

View reviewed changes

tdcmeehan merged commit 8b37cec into prestodb:master Dec 19, 2023

gupteaj deleted the test_transform branch December 20, 2023 01:57

hantangwangd mentioned this pull request Dec 20, 2023

Support year/month/day/hour transform when add partition column in Iceberg #21575

Merged

6 tasks

wanglinsong mentioned this pull request Feb 12, 2024

Add release notes for 0.286 #21906

Merged

64 tasks

Conversation

gupteaj commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hantangwangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hantangwangd left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan Dec 19, 2023

Choose a reason for hiding this comment

Uh oh!

hantangwangd Dec 19, 2023

Choose a reason for hiding this comment

Uh oh!

tdcmeehan Dec 19, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gupteaj commented Nov 3, 2023 •

edited

Loading

github-actions bot commented Nov 16, 2023 •

edited

Loading