Add UPDATE support for Iceberg #24281

ZacBlanco · 2024-12-18T22:04:57Z

Description

This PR adds support in the Iceberg connector for UPDATE operations.

Motivation and Context

Row-level table UPDATE support

Impact

Users can now set update_mode table property on Iceberg tables
Inserts and deletes now show operations as "overwrite" in the snapshot entries due to the new update implementation.
UPDATE <x> SET ... WHERE ... queries can now run successfully

Test Plan

Comprehensive set of unit tests for different tables and column types.

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
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

== RELEASE NOTES ==

Iceberg Changes
* Iceberg connector support for ``UPDATE`` SQL statements

steveburnett · 2025-01-03T17:55:21Z

Thanks for the release note! Formatting nits:

== RELEASE NOTES ==

Iceberg Changes
* Iceberg connector support for ``UPDATE`` SQL statements. :pr:`24281`

hantangwangd

Thanks for implementing this great feature. Some little problems and nits during the first rough inspection, and will take a detailed look in a couple of days.

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

hantangwangd

Thanks for this high-quality implementation, overall looks good to me, only some little nits.

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

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

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

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java

imjalpreet

Thanks for the PR! I'm sorry for the delay in reviewing it. I completed a partial review and will continue with the rest.

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

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

...o-iceberg/src/main/java/com/facebook/presto/iceberg/ConnectorPageSourceWithRowPositions.java

...o-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergPartitionInsertingPageSource.java

hantangwangd

Thanks for the whole work, looks good to me, only a couple of little nits.

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

imjalpreet

Thanks for the PR, almost LGTM, just a couple of nits.

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

...n/src/main/java/com/facebook/presto/sql/planner/optimizations/StreamPropertyDerivations.java

steveburnett · 2025-02-03T15:01:35Z

New release note guidelines as of last week: PR #24354 automatically adds links to this PR to the release notes. Please remove the manual PR link in the following format from the release note entries for this PR.

:pr:`12345`

I have updated the Release Notes Guidelines to remove the examples of manually adding the PR link.

steveburnett

Thanks for the doc! A few suggestions, looks good overall.

presto-docs/src/main/sphinx/connector/iceberg.rst

steveburnett

LGTM! (docs)

Pull updated branch, new local docs build, everything looks good. Thanks!

imjalpreet

LGTM, thanks for the PR @ZacBlanco

tdcmeehan

Please clean up the commits.

tdcmeehan

Just some nits, looks good

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

tdcmeehan · 2025-02-06T20:39:37Z

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

@@ -1237,8 +1256,8 @@ public static String metadataLocation(Table icebergTable)

    /**
     * Get the data location for target {@link Table},
-     *  considering iceberg table properties {@code WRITE_DATA_LOCATION}, {@code OBJECT_STORE_PATH} and {@code WRITE_FOLDER_STORAGE_LOCATION}
-     * */
+     * considering iceberg table properties {@code WRITE_DATA_LOCATION}, {@code OBJECT_STORE_PATH} and {@code WRITE_FOLDER_STORAGE_LOCATION}


I know the comments were improperly formatted before, but can you revert this? It would just muddle the git blame.

IntelliJ IDE likes to auto-format these...maybe I'll open a separate PR to apply formatting to the entire connector. This seems to happen quite often

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

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

...o-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergPartitionInsertingPageSource.java

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

Without this change, the UpdateOperator would throw an exception stating that there was no valid page source. This occurs because the driver which is responsible for setting the UpdateablePageSource never calls the proper method due to never receiving any inputs. This now handles the case where the page source is never set by returning an EmptySplitPageSource

This commit allows users to perform row-level updates when using the Iceberg connector with Java-based workers. This is achieved by improving on the IcebergUpdatablePageSource to implement the updateRows method. The implementation passes a generated row ID column as a field in the page required by updateRows. Then during updateRows, generated a positionDelete file entry for the row ID, and also writes the row's updated value to a new page sink for the newly updated data. These new files are then commited in a rowDelta transaction within the Iceberg connector metadata after processing is complete. Co-Authored-By: Nidhin Varghese <Nidhin.Varghese1@ibm.com> Co-Authored-By: Anoop V S <anoop.v.s@ibm.com>

prestodb-ci added the from:IBM PR from IBM label Dec 18, 2024

prestodb-ci requested review from a team, anandamideShakyan and ShahimSharafudeen and removed request for a team December 18, 2024 22:05

ZacBlanco force-pushed the upstream-iceberg-update branch from e56fc7c to f0a29a0 Compare December 20, 2024 18:28

ZacBlanco force-pushed the upstream-iceberg-update branch from 94cc4b3 to 4aa34a0 Compare December 31, 2024 16:56

ZacBlanco marked this pull request as ready for review January 2, 2025 19:07

ZacBlanco requested review from hantangwangd, a team, jaystarshot, feilong-liu and ClarenceThreepwood as code owners January 2, 2025 19:07

ZacBlanco requested a review from presto-oss January 2, 2025 19:07

ZacBlanco force-pushed the upstream-iceberg-update branch from 8961a59 to c66c259 Compare January 9, 2025 17:10

ZacBlanco requested review from imjalpreet and agrawalreetika January 9, 2025 17:10

hantangwangd reviewed Jan 13, 2025

View reviewed changes

hantangwangd requested changes Jan 26, 2025

View reviewed changes

imjalpreet reviewed Jan 26, 2025

View reviewed changes

ZacBlanco force-pushed the upstream-iceberg-update branch from 13e3782 to 52c43b4 Compare January 27, 2025 20:05

ZacBlanco requested review from imjalpreet and hantangwangd January 31, 2025 06:00

hantangwangd previously approved these changes Feb 2, 2025

View reviewed changes

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergUpdateablePageSource.java Outdated Show resolved Hide resolved

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergUpdateablePageSource.java Outdated Show resolved Hide resolved

imjalpreet reviewed Feb 2, 2025

View reviewed changes

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergUpdateablePageSource.java Outdated Show resolved Hide resolved

...n/src/main/java/com/facebook/presto/sql/planner/optimizations/StreamPropertyDerivations.java Show resolved Hide resolved

ZacBlanco dismissed hantangwangd’s stale review via f432a16 February 4, 2025 01:45

ZacBlanco force-pushed the upstream-iceberg-update branch from 52c43b4 to f432a16 Compare February 4, 2025 01:45

ZacBlanco requested a review from steveburnett as a code owner February 4, 2025 01:45

ZacBlanco requested a review from elharo as a code owner February 4, 2025 01:45

steveburnett requested changes Feb 4, 2025

View reviewed changes

steveburnett previously approved these changes Feb 4, 2025

View reviewed changes

hantangwangd previously approved these changes Feb 4, 2025

View reviewed changes

ZacBlanco requested a review from imjalpreet February 5, 2025 00:57

imjalpreet previously approved these changes Feb 5, 2025

View reviewed changes

tdcmeehan self-assigned this Feb 5, 2025

tdcmeehan requested changes Feb 6, 2025

View reviewed changes

ZacBlanco force-pushed the upstream-iceberg-update branch from 1a7292c to e3d8733 Compare February 6, 2025 19:13

tdcmeehan reviewed Feb 6, 2025

View reviewed changes

ZacBlanco dismissed stale reviews from hantangwangd, imjalpreet, and steveburnett via 3e8eacc February 6, 2025 23:37

ZacBlanco force-pushed the upstream-iceberg-update branch from e3d8733 to 3e8eacc Compare February 6, 2025 23:37

ZacBlanco and others added 2 commits February 7, 2025 11:22

ZacBlanco force-pushed the upstream-iceberg-update branch from 3e8eacc to cd4df4f Compare February 7, 2025 20:32

tdcmeehan approved these changes Feb 7, 2025

View reviewed changes

ZacBlanco merged commit 2005b0b into prestodb:master Feb 7, 2025
55 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UPDATE support for Iceberg #24281

Add UPDATE support for Iceberg #24281

ZacBlanco commented Dec 18, 2024 •

edited

Loading

steveburnett commented Jan 3, 2025

hantangwangd left a comment

hantangwangd left a comment

imjalpreet left a comment

hantangwangd left a comment

imjalpreet left a comment

steveburnett commented Feb 3, 2025

steveburnett left a comment

steveburnett left a comment

imjalpreet left a comment

tdcmeehan left a comment

tdcmeehan left a comment

tdcmeehan Feb 6, 2025

ZacBlanco Feb 6, 2025

Add UPDATE support for Iceberg #24281

Add UPDATE support for Iceberg #24281

Conversation

ZacBlanco commented Dec 18, 2024 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

steveburnett commented Jan 3, 2025

hantangwangd left a comment

Choose a reason for hiding this comment

hantangwangd left a comment

Choose a reason for hiding this comment

imjalpreet left a comment

Choose a reason for hiding this comment

hantangwangd left a comment

Choose a reason for hiding this comment

imjalpreet left a comment

Choose a reason for hiding this comment

steveburnett commented Feb 3, 2025

steveburnett left a comment

Choose a reason for hiding this comment

steveburnett left a comment

Choose a reason for hiding this comment

imjalpreet left a comment

Choose a reason for hiding this comment

tdcmeehan left a comment

Choose a reason for hiding this comment

tdcmeehan left a comment

Choose a reason for hiding this comment

tdcmeehan Feb 6, 2025

Choose a reason for hiding this comment

ZacBlanco Feb 6, 2025

Choose a reason for hiding this comment

ZacBlanco commented Dec 18, 2024 •

edited

Loading