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 DML operations on Delta tables with name column mapping #16183

Merged
merged 2 commits into from
Apr 18, 2023
Merged

Support DML operations on Delta tables with name column mapping #16183

merged 2 commits into from
Apr 18, 2023

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Feb 20, 2023 •
edited
Loading

Description

Relates to #12638

Release notes

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

# Delta Lake
* Add support for DML operations on tables with `name` column mapping. ({issue}`12638`)

@cla-bot cla-bot bot added the cla-signed label Feb 20, 2023
@ebyhr ebyhr mentioned this pull request Feb 20, 2023
Closed
10 tasks
@ebyhr ebyhr added the delta-lake Delta Lake connector label Feb 20, 2023
@ebyhr ebyhr self-assigned this Feb 20, 2023
@ebyhr ebyhr force-pushed the ebi/delta-brx-dml-column-mapping branch 2 times, most recently from c4b1df4 to ed5ac15 Compare February 27, 2023 05:48
@ebyhr ebyhr marked this pull request as ready for review February 28, 2023 06:01
@ebyhr ebyhr force-pushed the ebi/delta-brx-dml-column-mapping branch from ed5ac15 to 2dbee61 Compare February 28, 2023 11:31
@ebyhr ebyhr force-pushed the ebi/delta-brx-dml-column-mapping branch from 2dbee61 to 7d29b75 Compare March 13, 2023 09:34
@ebyhr ebyhr force-pushed the ebi/delta-brx-dml-column-mapping branch from 7d29b75 to 42d9b1a Compare March 14, 2023 03:28
@ebyhr ebyhr requested a review from findepi March 14, 2023 22:36
@findepi
Copy link
Member

findepi commented Mar 16, 2023

@alexjo2144 PTAL

@findinpath findinpath mentioned this pull request Mar 16, 2023
Merged
@ebyhr ebyhr force-pushed the ebi/delta-brx-dml-column-mapping branch 3 times, most recently from 0db4947 to b750ff8 Compare April 6, 2023 01:40
krvikash
krvikash approved these changes Apr 7, 2023
View reviewed changes
Copy link
Contributor

@krvikash krvikash left a comment

Choose a reason for hiding this comment

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

LGTM

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved
@ebyhr ebyhr force-pushed the ebi/delta-brx-dml-column-mapping branch from b750ff8 to a6141a3 Compare April 10, 2023 05:39
@ebyhr
Copy link
Member Author

ebyhr commented Apr 11, 2023

@alexjo2144 Could you take a look at this PR when you have the time?

alexjo2144
alexjo2144 reviewed Apr 11, 2023
View reviewed changes
Copy link
Member

@alexjo2144 alexjo2144 left a comment

Choose a reason for hiding this comment

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

Are we using the plain names in our NDV statistics or the mapped names? Thinking about the same where a column is dropped and then re-added with the same name. Stats should be cleared.

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved
@alexjo2144
Copy link
Member

Similarly, do the JSON and Checkpoint stats use human readable names or mapped names?

@ebyhr
Copy link
Member Author

ebyhr commented Apr 12, 2023 •
edited
Loading

JSON and Checkpoint stats use mapped names, but extended stats uses human-readable names. Looking into the details.

@ebyhr ebyhr force-pushed the ebi/delta-brx-dml-column-mapping branch from a6141a3 to 51e1623 Compare April 12, 2023 05:53
@findinpath
Copy link
Contributor

Please rebase to handle conflicts with master.

@ebyhr ebyhr force-pushed the ebi/delta-brx-dml-column-mapping branch 3 times, most recently from 850a7d3 to 5f8a0d1 Compare April 13, 2023 10:48
@ebyhr
Copy link
Member Author

ebyhr commented Apr 13, 2023

Added the test case about changing column mapping mode. Databricks supports changing to name from none, but the extended stats doesn't restore the old stats because the physical names are different between old and new.

@ebyhr
Copy link
Member Author

ebyhr commented Apr 14, 2023

I will rebase this PR after merging #16905.

@ebyhr ebyhr force-pushed the ebi/delta-brx-dml-column-mapping branch 2 times, most recently from 90fc238 to 76fb5eb Compare April 17, 2023 01:31
@ebyhr
Copy link
Member Author

ebyhr commented Apr 17, 2023

maven-checks (20) hit #11131

findepi
findepi reviewed Apr 17, 2023
View reviewed changes
Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

last fixup lgtm

plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Show resolved Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java Outdated Show resolved Hide resolved
@ebyhr ebyhr force-pushed the ebi/delta-brx-dml-column-mapping branch from 76fb5eb to 93e01d6 Compare April 17, 2023 23:06
@ebyhr ebyhr force-pushed the ebi/delta-brx-dml-column-mapping branch from 93e01d6 to 47a0080 Compare April 18, 2023 00:59
@ebyhr ebyhr merged commit 7d820eb into master Apr 18, 2023
@ebyhr ebyhr deleted the ebi/delta-brx-dml-column-mapping branch April 18, 2023 04:31
@ebyhr ebyhr mentioned this pull request Apr 18, 2023
Closed
@github-actions github-actions bot added this to the 414 milestone Apr 18, 2023
@colebow colebow mentioned this pull request Apr 19, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pajaks pajaks pajaks left review comments

@findinpath findinpath findinpath approved these changes

@alexjo2144 alexjo2144 alexjo2144 left review comments

@findepi findepi findepi approved these changes

@krvikash krvikash krvikash approved these changes

@homar homar Awaiting requested review from homar

Assignees

@ebyhr ebyhr

Labels
cla-signed delta-lake Delta Lake connector
Milestone
414
Development

Successfully merging this pull request may close these issues.
6 participants
@ebyhr @findepi @alexjo2144 @findinpath @krvikash @pajaks