-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add support for absolute path in Delta Lake connector #17038
Add support for absolute path in Delta Lake connector #17038
Conversation
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: 300070018.
|
@vinay-kl pls follow the guideline https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages for commit messages. |
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeSplitManager.java
Outdated
Show resolved
Hide resolved
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: 300070018.
|
Shallow Cloned
tables
@ebyhr @alexjo2144 can you please review this PR |
Small checkstyle issues in your PR
Address them and do a build before pushing:
At this stage do consider a rebase.
|
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: 300070018.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for proposing a fix! There are a couple other places that use AddFileEntry.getPath()
that probably also need an update but I don't think any others are used on the read path so we can look at them separately.
@findepi @findinpath we probably need to think through if all table operations make sense on clones. DML, Analyze, optimize, vacuum, etc.
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeSplitManager.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeSplitManager.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeSplitManager.java
Outdated
Show resolved
Hide resolved
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: 300070018.
|
1 similar comment
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: 300070018.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeSplitManager.java
Outdated
Show resolved
Hide resolved
@vinay-kl the CLA bot doesn't seem happy, have you filled one out? |
@alexjo2144 I have, The CLA was accepted and confirmation mail from martin was received yesterday, I'm not sure why the CLA issue still prevails. |
Awesome. In that case, can you double check the couple steps listed in the bot's messages to make sure that your commits are tagged appropriately? |
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: 300070018.
|
sorry, by mistakingly i closed this PR and don't suppose how to open this.. will raise another PR. Extremely sorry for inconvenience @alexjo2144 @findinpath |
Hi @vinay-kl, Reopened the PR. |
8978a0a
to
ec3d68e
Compare
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: 300070018.
|
@krvikash Thank you so much for re-opening the PR. |
Your account isn't yet registered in https://github.com/trinodb/cla. Please wait for a while. |
@mosabua I will continue working on this PR, will submit the latest commit changes soon. |
d2f563d
to
1ea810e
Compare
...in/java/io/trino/tests/product/deltalake/TestDeltaLakeDatabricksCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
...in/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/transactionlog/AddFileEntry.java
Outdated
Show resolved
Hide resolved
1ea810e
to
12102c7
Compare
...in/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/transactionlog/AddFileEntry.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeSplitManager.java
Outdated
Show resolved
Hide resolved
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
@vinay-kl pls rebase on |
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeSplitManager.java
Show resolved
Hide resolved
12102c7
to
4ed0460
Compare
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Show resolved
Hide resolved
dropDeltaTableWithRetry("default." + clonedTable); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you please add a simple test showcasing the following scenario:
- create table t1
- spark: create table t2 shallow clone t1
- ensure that t2 and t1 have the same content
- trino: drop table t2
- ensure that the t1 table has the content intact
- trino: drop table t1
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
4ed0460
to
7a8dd02
Compare
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % open nits
7a8dd02
to
ff0f200
Compare
/test-with-secrets sha=ff0f20084d0e10b159893dd577b3d094e4792864 |
The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/8507516113 |
...in/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/transactionlog/AddFileEntry.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/procedure/VacuumProcedure.java
Outdated
Show resolved
Hide resolved
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
row(2, "a", "update_postimage", 2L), | ||
row(2, "b", "update_preimage", 2L), | ||
row(3, "b", "update_postimage", 2L)); | ||
// table_changes function from trino isn't considering `base table inserts on shallow cloned table` as CDF as of v422 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as of v422
It's different from the current version. I would recommend removing.
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
...sts/src/main/java/io/trino/tests/product/deltalake/TestDeltaLakeCloneTableCompatibility.java
Outdated
Show resolved
Hide resolved
ff0f200
to
3de4fdc
Compare
Description
Support reading absolute paths from the Delta transaction log
Additional context and related issues
Fixes #17011
Release notes
(x) Release notes are required, with the following suggested text: