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

Remove support for legacy Parquet writer in Delta Lake #15436

Merged
merged 3 commits into from
Jan 31, 2023
Merged

Remove support for legacy Parquet writer in Delta Lake #15436

merged 3 commits into from
Jan 31, 2023

Conversation

pajaks
Copy link
Member

@pajaks pajaks commented Dec 16, 2022 •
edited by ebyhr
Loading

Description

Remove support for legacy Parquet writer in Delta Lake connector

Additional context and related issues

Release notes

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

# Delta Lake
* Remove support for non-optimzied Parquet writer.

@cla-bot
Copy link

cla-bot bot commented Dec 16, 2022

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 cla@trino.io. 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
ebyhr reviewed Dec 16, 2022
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Could you update below places?

  • delta-lake.rst
  • BaseDeltaLakeMinioConnectorTest.testTargetMaxFileSize
  • io.trino.tests.product.deltalake.TestDeltaLakeDatabricksInsertCompatibility#testCompression - Please note that this test wasn't trigger in CI because PR came from forked repository

.../src/test/java/io/trino/plugin/deltalake/TestDeltaLakeCreateTableStatisticsLegacyWriter.java Show resolved Hide resolved
@cla-bot cla-bot bot added the cla-signed label Dec 21, 2022
@pajaks pajaks marked this pull request as ready for review December 22, 2022 11:31
@pajaks pajaks marked this pull request as draft December 22, 2022 16:34
@ebyhr
Copy link
Member

ebyhr commented Jan 3, 2023 •
edited
Loading

Could you rebase on upstream to resolve conflicts?

@ebyhr ebyhr marked this pull request as ready for review January 3, 2023 23:44
@pajaks pajaks changed the title Remove support for legacy Parquet writer in Delta Lake DO NOT MERGE Remove support for legacy Parquet writer in Delta Lake Jan 4, 2023
@electrum
Copy link
Member

electrum commented Jan 4, 2023

We should wait for #9142 to be completed.

@pajaks pajaks requested review from findepi and homar January 5, 2023 14:08
@findepi
Copy link
Member

findepi commented Jan 5, 2023

We should wait for #9142 to be completed.

We currently use the new writer and it currently doesn't support LZ4.
Are you OK, @electrum , with users still using the old writer whenever they want to compress with LZ4?
I am concerned this means we keep supporting and (testing for) both writers.

Wdyt?

@electrum
Copy link
Member

electrum commented Jan 5, 2023

If the new writer is the default, then it's probably fine to merge this. We can implement LZ4 if anyone needs that.

@pajaks pajaks changed the title DO NOT MERGE Remove support for legacy Parquet writer in Delta Lake Remove support for legacy Parquet writer in Delta Lake Jan 9, 2023
@ebyhr
Copy link
Member

ebyhr commented Jan 27, 2023

Rebased on upstream to resolve conflicts on docs.

@pajaks
Copy link
Member Author

pajaks commented Jan 30, 2023

rebase on master to use CI fix #15879

@ebyhr
Copy link
Member

ebyhr commented Jan 30, 2023

/test-with-secrets sha=c1a989b99bd28382a8bdf89ab39ed44989c227db

@pajaks
Copy link
Member Author

pajaks commented Jan 31, 2023

suite-7-non-generic hit #14441

@ebyhr
Copy link
Member

ebyhr commented Jan 31, 2023

All tests passed in https://github.com/trinodb/trino/actions/runs/4049319103/jobs/6965546618. Let me merge now.

@ebyhr ebyhr merged commit dbc87c7 into trinodb:master Jan 31, 2023
@ebyhr ebyhr mentioned this pull request Jan 31, 2023
Closed
@colebow colebow added this to the 407 milestone Jan 31, 2023
@colebow colebow mentioned this pull request Jan 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebyhr ebyhr ebyhr approved these changes

@findepi findepi Awaiting requested review from findepi

@homar homar Awaiting requested review from homar

Assignees
No one assigned
Labels
cla-signed docs
Milestone
407
Development

Successfully merging this pull request may close these issues.
5 participants
@pajaks @ebyhr @electrum @findepi @colebow