Add support for reading v2 row level deletes in Iceberg connector by wypb · Pull Request #21189 · prestodb/presto

wypb · 2023-10-19T11:13:55Z

Description

Add support for reading v2 row level deletes in Iceberg connector

Cherry-pick of trinodb/trino#11642
Cherry-pick of trinodb/trino#13219
Cherry-pick of trinodb/trino#13395

Motivation and Context

Impact

Test Plan

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
* Add support for reading v2 row level deletes in Iceberg connector

yingsu00 · 2023-10-19T23:55:02Z

@wypb Thank you for submitting the PR! THere're some test failures in Iceberg. Can you please fix them?

github-actions · 2023-10-20T01:20:06Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff c361c45...f6b2b68.

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

wypb · 2023-10-20T05:48:15Z

Hi @yingsu00 Thank you for your reply. I've fixed these failed test sets.

yingsu00 · 2023-10-20T21:53:59Z

@wypb I helped resolving the conflict but it results in a separate commit "Merge branch 'master' into iceberg_deletefile" Can you please squash it?

wypb · 2023-10-23T02:22:26Z

HI @yingsu00 I have squashed the this commit and my branch has been synchronized with the latest code of the master branch.

tdcmeehan

Some minor comments, will finish review after I return on Wednesday.

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

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

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

wypb · 2023-10-24T01:41:51Z

Hi @tdcmeehan I modified the corresponding code according to the review comments, Thank you for your review

steveburnett · 2023-11-01T16:13:07Z

@wypb I think we should set the default value iceberg.enable-merge-on-read-mode to true in this PR. Thoughts?

If this happens, please update the doc in line 213 of iceberg.rst appropriately.

wypb · 2023-11-02T06:12:40Z

I think we should set the default value iceberg.enable-merge-on-read-mode to true in this PR. Thoughts?

@tdcmeehan I have read the Iceberg document, what you say is right, I will set the default value of iceberg.enable-merge-on-read-mode to true. thank you.

wypb · 2023-11-02T06:21:09Z

Hi @steveburnett I have updated the corresponding documents, please help me take a look again, thank you.

wypb · 2023-11-02T11:06:42Z

HI @tdcmeehan I synchronized the latest code of the master branch, and then several test cases related to delete file failed.

java.lang.NoSuchMethodError: org.apache.parquet.hadoop.ParquetFileWriter.<init>(Lorg/apache/parquet/io/OutputFile;Lorg/apache/parquet/schema/MessageType;Lorg/apache/parquet/hadoop/ParquetFileWriter$Mode;JIIIZLorg/apache/parquet/crypto/InternalFileEncryptor;)V

	at org.apache.iceberg.parquet.ParquetWriter.ensureWriterInitialized(ParquetWriter.java:111)
	at org.apache.iceberg.parquet.ParquetWriter.flushRowGroup(ParquetWriter.java:210)
	at org.apache.iceberg.parquet.ParquetWriter.close(ParquetWriter.java:254)
	at org.apache.iceberg.deletes.EqualityDeleteWriter.close(EqualityDeleteWriter.java:78)
	at com.facebook.presto.iceberg.IcebergDistributedTestBase.writeEqualityDeleteToNationTable(IcebergDistributedTestBase.java:583)
	at com.facebook.presto.iceberg.IcebergDistributedTestBase.testTableWithEqualityDelete(IcebergDistributedTestBase.java:466)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:135)
	at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:673)
	at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:220)
	at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
	at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:945)
	at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:193)
	at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
	at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
	at java.util.ArrayList.forEach(ArrayList.java:1259)
	at org.testng.TestRunner.privateRun(TestRunner.java:808)
	at org.testng.TestRunner.run(TestRunner.java:603)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:429)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:423)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:383)
	at org.testng.SuiteRunner.run(SuiteRunner.java:326)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:95)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1249)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1169)
	at org.testng.TestNG.runSuites(TestNG.java:1092)
	at org.testng.TestNG.run(TestNG.java:1060)
	at com.intellij.rt.testng.IDEARemoteTestNG.run(IDEARemoteTestNG.java:66)
	at com.intellij.rt.testng.RemoteTestNGStarter.main(RemoteTestNGStarter.java:109)

It was because Iceberg was recently upgraded to 1.4.1 (#21230), and the EqualityDeleteWriter and PositionDeleteWriter rely on the new ParquetFileWriter method added in parquet 1.13.1, but the current parquet dependency version of Presto is 1.12.0, So I need to upgrade the Parquet version of presto and presto-hive-apache projects. Any Thoughts?

tdcmeehan · 2023-11-02T14:34:25Z

@wypb let's update the Parquet dependency in Presto and presto-hive-apache. The latter release will take some time, but I think we'll be able to do it in the next few days.
Let me know if you'd like to raise the PR for those. @imjalpreet is following up on enabling a presto-hive-apache release.

s-akhtar-baig · 2023-11-02T14:39:49Z

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

Please update IcebergDistributedSmokeTestBase::testMergeOnReadDisabled to set the session property.

Sure, I will fix this. thank you.

@s-akhtar-baig Fixed.

Perfect, thank you!

imjalpreet · 2023-11-02T14:52:59Z

@tdcmeehan I just saw for this we just have a dependency on presto-hive-apache which is not blocked and can be released after we update the parquet dependency.

Release of presto-hadoop-apache2 has been blocked which I will be unblocking soon. But this PR is not dependent on that.

tdcmeehan · 2023-11-02T14:56:51Z

@imjalpreet you're right.

To clear things up: let's try to directly make the updates to the Parquet dependency. Myself and @wanglinsong can cut the release for presto-hive-apache. @wypb , please let me know if you would like to raise the PRs bumping the dependency.

wypb · 2023-11-02T15:50:12Z

@tdcmeehan I will submit a new PR to upgrade the version of parquet to 1.13.1.

BTW, This PR should rely on presto-hive-apache to upgrade Parquet to 1.13.x, because the org.apache.parquet.hadoop.ParquetFileWriter class is in parquet-hadoop module, and Presto uses this class through presto-hive-apache, which itself does not directly depend on parquet-hadoop. Therefore, we need to upgrade the Parquet dependencies of Presto and presto-hive-apache at the same time before merging this PR. @imjalpreet @tdcmeehan

steveburnett · 2023-11-02T17:08:56Z

Hi @steveburnett I have updated the corresponding documents, please help me take a look again, thank you.

Hi @wypb, I pulled the updated branch, made a local build of the docs, and everything in the docs looks great. Thank you!

ChunxuTang

Went through the code again and it looks good to me. @wypb Thanks for your updates!
We probably could merge this PR after the version issue is resolved.

tdcmeehan · 2023-11-08T16:01:38Z

@wypb will you be creating the PR to bump the dependency in presto-hive-apache?

wypb · 2023-11-09T05:43:28Z

@tdcmeehan I've already created one(prestodb/presto-hive-apache#61), please help me review this, thank you.

wypb · 2023-12-05T11:28:31Z

HI @tdcmeehan I have synchronized the latest code of the master branch, and now the test cases can be run. Could you help me take a look again? thank you.

tdcmeehan · 2023-12-05T16:27:35Z

presto-hive/src/main/java/com/facebook/presto/hive/orc/OrcBatchPageSource.java

+        this(recordReader, orcDataSource, columns, typeManager, systemMemoryContext, stats, runtimeStats, nCopies(columns.size(), false));
+    }
+
+    public OrcBatchPageSource(


Can you create an issue to add whatever is necessary for the corresponding selective page source?

tdcmeehan · 2023-12-05T16:29:21Z

Thank you for following through this long PR and chasing all the loose ends @wypb!

wypb requested a review from a team as a code owner October 19, 2023 11:13

wypb requested a review from presto-oss October 19, 2023 11:13

wypb force-pushed the iceberg_deletefile branch from e340af4 to 4b3bed6 Compare October 19, 2023 11:18

yingsu00 requested review from tdcmeehan and yingsu00 October 19, 2023 23:51

wypb force-pushed the iceberg_deletefile branch 2 times, most recently from 966d45a to 2fc3cf7 Compare October 20, 2023 01:19

wypb force-pushed the iceberg_deletefile branch from 595a2e1 to 588d403 Compare October 20, 2023 04:06

wypb force-pushed the iceberg_deletefile branch 3 times, most recently from 5d605fb to 1bc4744 Compare October 20, 2023 07:58

tdcmeehan self-assigned this Oct 20, 2023

tdcmeehan requested a review from ChunxuTang October 20, 2023 20:15

wypb force-pushed the iceberg_deletefile branch 4 times, most recently from f7508d2 to c039bf2 Compare October 23, 2023 01:10

tdcmeehan linked an issue Oct 23, 2023 that may be closed by this pull request

Iceberg MoR support - reads #20492

Closed

tdcmeehan reviewed Oct 24, 2023

View reviewed changes

wypb force-pushed the iceberg_deletefile branch from 3360bf8 to 719024c Compare October 24, 2023 01:35

wypb force-pushed the iceberg_deletefile branch 4 times, most recently from a4ab09b to 45d83d4 Compare October 25, 2023 11:15

wypb force-pushed the iceberg_deletefile branch from 1c22141 to 48471fa Compare November 2, 2023 06:19

wypb force-pushed the iceberg_deletefile branch from 79c8fe6 to 81b6f21 Compare November 2, 2023 08:24

s-akhtar-baig reviewed Nov 2, 2023

View reviewed changes

ChunxuTang approved these changes Nov 2, 2023

View reviewed changes

wypb mentioned this pull request Nov 3, 2023

Upgrade parquet to 1.13.1 #21301

Merged

6 tasks

wypb force-pushed the iceberg_deletefile branch from 878a729 to 0852c49 Compare November 3, 2023 01:48

wypb mentioned this pull request Nov 9, 2023

Upgrade parquet to 1.13.1 prestodb/presto-hive-apache#61

Merged

wypb mentioned this pull request Nov 21, 2023

Update hive-apache to 3.0.0-10 #21428

Merged

6 tasks

wypb force-pushed the iceberg_deletefile branch 2 times, most recently from 751ce06 to 71ac549 Compare December 5, 2023 08:00

Add support for reading v2 row level deletes in Iceberg connector

f6b2b68

wypb force-pushed the iceberg_deletefile branch from 860998f to f6b2b68 Compare December 5, 2023 09:02

tdcmeehan approved these changes Dec 5, 2023

View reviewed changes

tdcmeehan merged commit 61c7980 into prestodb:master Dec 5, 2023

wypb deleted the iceberg_deletefile branch December 6, 2023 00:50

agrawalreetika mentioned this pull request Dec 22, 2023

Update Iceberg Document for SELECT Sql Support #21596

Merged

6 tasks

wanglinsong mentioned this pull request Feb 12, 2024

Add release notes for 0.286 #21906

Merged

64 tasks

Conversation

wypb commented Oct 19, 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!

yingsu00 commented Oct 19, 2023

Uh oh!

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

Uh oh!

Uh oh!

wypb commented Oct 20, 2023

Uh oh!

yingsu00 commented Oct 20, 2023

Uh oh!

wypb commented Oct 23, 2023

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wypb commented Oct 24, 2023

Uh oh!

steveburnett commented Nov 1, 2023

Uh oh!

wypb commented Nov 2, 2023

Uh oh!

wypb commented Nov 2, 2023

Uh oh!

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

Uh oh!

Uh oh!

tdcmeehan commented Nov 2, 2023

Uh oh!

s-akhtar-baig Nov 2, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wypb Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

s-akhtar-baig Nov 3, 2023

Choose a reason for hiding this comment

Uh oh!

imjalpreet commented Nov 2, 2023

Uh oh!

tdcmeehan commented Nov 2, 2023

Uh oh!

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

Uh oh!

Uh oh!

steveburnett commented Nov 2, 2023

Uh oh!

ChunxuTang left a comment

Choose a reason for hiding this comment

Uh oh!

tdcmeehan commented Nov 8, 2023

Uh oh!

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

Uh oh!

Uh oh!

wypb commented Dec 5, 2023

Uh oh!

tdcmeehan Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

tdcmeehan commented Dec 5, 2023

Uh oh!

Reviewers

Assignees

Labels

wypb commented Oct 19, 2023 •

edited

Loading

github-actions bot commented Oct 20, 2023 •

edited

Loading

wypb commented Nov 2, 2023 •

edited

Loading

wypb Nov 2, 2023 •

edited

Loading

wypb commented Nov 2, 2023 •

edited

Loading

wypb commented Nov 9, 2023 •

edited

Loading