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

Support automatic type coercion in Delta table creation #20814

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Support automatic type coercion in Delta table creation #20814

merged 1 commit into from
Mar 21, 2024

Conversation

krvikash
Copy link
Contributor

@krvikash krvikash commented Feb 22, 2024 •
edited

Description

Fixes #19336

For trino type TimestampType automatic coercion to TIMESTAMP_MICROS.

Related to #13981

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
(X) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Delta lake
* Support automatic type coercion for timestamp type in Delta table creation. ({issue}`19336`)

@cla-bot cla-bot bot added the cla-signed label Feb 22, 2024
wendigo
wendigo reviewed Feb 22, 2024
View reviewed changes
plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeConnectorTest.java Outdated
}

@ParameterizedTest
@MethodSource("timestampPrecisionOnCreateTableProvider")
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't use parametrized tests afaik. please loop over values in the method itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that Iceberg is already using parametrized tests db9f1b9

Copy link
Member

Choose a reason for hiding this comment

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

Some additional context on not using Parameterized tests #19532 (comment).

@martint / @dain In this case even if we replace the parametrized tests with a for loop we still won't be able to properly abstract the test data from the testing code - Or can we replace them with a TypeMapping like tests here ? Which allows us to test multiple input cases and treats them as a single row

@github-actions github-actions bot added the delta-lake Delta Lake connector label Feb 22, 2024
@krvikash krvikash force-pushed the krvikash/delta-auto-coercion-for-timestamp branch 2 times, most recently from d84b254 to c8f26e8 Compare February 23, 2024 11:05
@krvikash
Copy link
Contributor Author

Thanks @wendigo, @Praveen2112 for the review. Addressed comments.

Praveen2112
Praveen2112 reviewed Feb 23, 2024
View reviewed changes
Copy link
Member

@Praveen2112 Praveen2112 left a comment

Choose a reason for hiding this comment

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

Overall LGTM. But I wish to get a feedback from @dain / @martint as even implementing them via for loop would still move the test cases away from the actual tests.

@findinpath
Copy link
Contributor

cc @hovaesco , @mdesmet FYI this development will likely unblock some dbt use-cases on Delta Lake

@findinpath
Copy link
Contributor

findinpath commented Feb 23, 2024 •
edited

I wish to get a feedback from @dain / @martint as even implementing them via for loop would still move the test cases away from the actual tests.

@Praveen2112 Please see the pattern for handling "parameterized" tests in 11391fc - they follow the same pattern as this PR.

@krvikash krvikash force-pushed the krvikash/delta-auto-coercion-for-timestamp branch from c8f26e8 to a77dc7b Compare February 28, 2024 11:01
@krvikash
Copy link
Contributor Author

Thanks @Praveen2112 , @findinpath for the review. Provided explicit test datas for test instead for using for loop.

@krvikash krvikash force-pushed the krvikash/delta-auto-coercion-for-timestamp branch from a77dc7b to 12deb3b Compare February 28, 2024 17:29
@krvikash
Copy link
Contributor Author

Thanks for the review @martint. Addressed comments.

@krvikash krvikash requested review from findepi and ebyhr March 7, 2024 06:46
@krvikash krvikash force-pushed the krvikash/delta-auto-coercion-for-timestamp branch from 12deb3b to 0d63cbf Compare March 7, 2024 06:47
@krvikash
Copy link
Contributor Author

krvikash commented Mar 7, 2024

(rebased with master)

@findinpath findinpath requested a review from martint March 7, 2024 09:58
@martint martint dismissed their stale review March 8, 2024 01:18

Comments have been addressed

@krvikash krvikash force-pushed the krvikash/delta-auto-coercion-for-timestamp branch from 0d63cbf to 0410706 Compare March 8, 2024 13:36
@krvikash krvikash force-pushed the krvikash/delta-auto-coercion-for-timestamp branch 2 times, most recently from e04a5fc to b2c9b6c Compare March 15, 2024 07:52
@krvikash
Copy link
Contributor Author

Thanks @ebyhr for the review. Addressed comments.

@krvikash krvikash force-pushed the krvikash/delta-auto-coercion-for-timestamp branch from b2c9b6c to 3976509 Compare March 15, 2024 07:58
@krvikash
Copy link
Contributor Author

(minor table name change)

@krvikash krvikash force-pushed the krvikash/delta-auto-coercion-for-timestamp branch from 3976509 to 6f6c10b Compare March 15, 2024 09:44
@krvikash krvikash requested a review from ebyhr March 19, 2024 06:21
@krvikash krvikash force-pushed the krvikash/delta-auto-coercion-for-timestamp branch from 6f6c10b to 6a20716 Compare March 21, 2024 08:09
@krvikash
Copy link
Contributor Author

@ebyhr Addressed comments.

@Praveen2112 Praveen2112 merged commit e9adf94 into trinodb:master Mar 21, 2024
24 checks passed
@Praveen2112
Copy link
Member

@krvikash Thanks for fixing this. Can you update the PR description to reflect the release notes.

@github-actions github-actions bot added this to the 443 milestone Mar 21, 2024
@krvikash krvikash deleted the krvikash/delta-auto-coercion-for-timestamp branch March 21, 2024 15:43
@mosabua
Copy link
Member

mosabua commented Mar 21, 2024

Added RN with minor rewording.

@mosabua mosabua mentioned this pull request Mar 21, 2024
Merged
@krvikash krvikash mentioned this pull request Mar 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wendigo wendigo wendigo left review comments

@ebyhr ebyhr ebyhr approved these changes

@Praveen2112 Praveen2112 Praveen2112 approved these changes

@findinpath findinpath findinpath approved these changes

@vlad-lyutenko vlad-lyutenko vlad-lyutenko approved these changes

@pajaks pajaks Awaiting requested review from pajaks

@findepi findepi Awaiting requested review from findepi

@martint martint Awaiting requested review from martint

Assignees
No one assigned
Labels
cla-signed delta-lake Delta Lake connector
Milestone
443
Development

Successfully merging this pull request may close these issues.

Add coercion for timestamp in Delta Lake connector
8 participants
@krvikash @findinpath @Praveen2112 @mosabua @martint @wendigo @ebyhr @vlad-lyutenko