Support row level deletes in the Iceberg connector#11886

Merged

findepi merged 3 commits intotrinodb:masterfrom

alexjo2144:iceberg/row-level-deletes-write

Apr 25, 2022

Member

alexjo2144 commented Apr 8, 2022 •

edited

Loading

Description

Support removing individual rows by writing positional delete files
compatible with v2 of the Iceberg specification.

Is this change a fix, improvement, new feature, refactoring, or other?

New feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Iceberg connector

How would you describe this change to a non-technical end user or system administrator?

Allow deleting individual rows from an Iceberg table

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
(x) Documentation PR is available with #12061
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Iceberg
* Support row level deletes following the Iceberg v2 specification for merge-on-read.

cla-bot bot added the cla-signed label

alexjo2144 added the WIP label

alexjo2144 force-pushed the iceberg/row-level-deletes-write branch from 15df4e5 to b096e2d Compare

April 8, 2022 21:44

alexjo2144 mentioned this pull request

Support setting Iceberg table format version #11880

Merged

alexjo2144 force-pushed the iceberg/row-level-deletes-write branch from b096e2d to 6ab810c Compare

April 11, 2022 18:40

github-actions bot added the docs label

alexjo2144 force-pushed the iceberg/row-level-deletes-write branch from 6ab810c to edfb55a Compare

April 12, 2022 15:49

github-actions bot added the tests:hive label

alexjo2144 force-pushed the iceberg/row-level-deletes-write branch 2 times, most recently from 5a2e8da to a28a32c Compare

April 12, 2022 20:42

findepi mentioned this pull request

Support Iceberg row-level delete and update #10075

Closed

Member

findepi commented Apr 13, 2022

@alexjo2144 can you PTAL at the CI output?

alexjo2144 commented

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated Show resolved Hide resolved

alexjo2144 force-pushed the iceberg/row-level-deletes-write branch 2 times, most recently from 1723c49 to 8ebba99 Compare

April 13, 2022 20:18

alexjo2144 requested review from findepi and losipiuk

April 13, 2022 20:18

alexjo2144 removed the WIP label

Member Author

alexjo2144 commented Apr 13, 2022

Should be ready for review now

alexjo2144 commented

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergPageSourceProvider.java Outdated Show resolved Hide resolved

alexjo2144 commented

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated Show resolved Hide resolved

alexjo2144 commented

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated

Member Author

alexjo2144 Apr 14, 2022 •

edited

Loading

We likely should have some follow up here. If an entire file is deleted this code will write a position delete file marking every row as deleted. Instead I think we could use transaction.newDelete()

That's kinda hard to do right now though, because a file may be separated into several Splits, handled by different workers.

Member Author

alexjo2144 Apr 20, 2022

alexjo2144 force-pushed the iceberg/row-level-deletes-write branch from 25a3e3a to 4a24328 Compare

April 14, 2022 20:57

alexjo2144 commented

View reviewed changes

...roduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java Outdated Show resolved Hide resolved

findepi reviewed

View reviewed changes

docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

findepi force-pushed the iceberg/row-level-deletes-write branch 2 times, most recently from 87d5d67 to dde16e6 Compare

April 15, 2022 07:26

findepi reviewed

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated

Comment on lines 953 to 969

Member

findepi Apr 15, 2022

if deleteMode==null (no specific mode request), it seems correct to carry out the delete operation.
-- we're fulfilling the v2 spec, and not violating any explicit request.
Let's just do this change.

furthermore, when delete mode is set !- merge-on-read. Can this be for performance reasons?
If so, that would be "a soft preference", perhaps set for Spark's benefit (which supports multiple delete modes). In such case ignoring it and carrying out the delete would be OKish too (best option currently possible).

Can this be set for compatibility reasons? I don't know.

cc @electrum @phd3 @rdblue

Member

findepi Apr 19, 2022

Actually, i came to a conclusion that this should be viewed as a tuning property only, i.e. advisory. We don't support two modes, so there is nothing to tune, we just just proceed.

Contributor

rdblue Apr 19, 2022

I agree that this should be advisory. This is a preferred mode.

If a user were to need to enforce this, I think it would only be for the eager rewrite / copy-on-write mode. But that can be enforced by using a v1 table instead of a v2.

homar reviewed

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java Outdated Show resolved Hide resolved

alexjo2144 force-pushed the iceberg/row-level-deletes-write branch from dde16e6 to 5899dd3 Compare

April 15, 2022 14:05

Member

findepi commented Apr 21, 2022

Added a change in TestIcebergPartitionEvolution#testDroppedPartitionField to use v1.

Member

findepi commented Apr 21, 2022

(forced re-run after #11980 merged)

alexjo2144 commented

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergTableHandle.java Outdated Show resolved Hide resolved

alexjo2144 force-pushed the iceberg/row-level-deletes-write branch 2 times, most recently from 4a3c2b5 to 589b670 Compare

April 21, 2022 14:53

rdblue reviewed

View reviewed changes

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergFileWriterFactory.java Outdated

Contributor

rdblue Apr 21, 2022

I think this is unclear. The format is not supported by Trino. But Avro, for example, is supported by Iceberg.

Member Author

alexjo2144 Apr 21, 2022

Removed "for Iceberg".

alexjo2144 force-pushed the iceberg/row-level-deletes-write branch 2 times, most recently from 2653df7 to 4cf6070 Compare

April 21, 2022 19:21

alexjo2144 commented

View reviewed changes

.../trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergMinioConnectorSmokeTest.java Outdated

Member Author

alexjo2144 Apr 21, 2022

GH link: #11713

alexjo2144 force-pushed the iceberg/row-level-deletes-write branch 3 times, most recently from b5f70de to 413578d Compare

April 22, 2022 13:45

findepi mentioned this pull request

Test: Support row level deletes in the Iceberg connector #12105

Closed

Member

findepi commented Apr 22, 2022

Test PR: #12105 (cc @ilfrin @nineinchnick for potentially streamlining this)


          Use Iceberg v2 by default

1c150b4

alexjo2144 force-pushed the iceberg/row-level-deletes-write branch from 413578d to d83a9d4 Compare

April 22, 2022 17:36

Member Author

alexjo2144 commented Apr 22, 2022

Rebased for merge conflicts

findepi reviewed

View reviewed changes

...rino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMetastoreAccessOperations.java Outdated

    
                      assertMetastoreInvocations("SELECT * FROM test_select_from",

                              ImmutableMultiset.builder()

                                      .add(GET_TABLE)

                                      .addCopies(GET_TABLE, 2)

Member

findepi Apr 25, 2022

Why does "Improve Iceberg commit to HMS" change the invocation counts on the read path?

Member Author

alexjo2144 Apr 25, 2022

The first one is TrinoHiveCatalog getting the table to check if the table is a view/materialized view. The second call is AbstractMetastoreTableOperations loading the Iceberg table, when it does that the cache is invalidated to get the most recent Snapshot.

I couldn't find a good way to invalidate the cache only during commit retries and not during initial table loading, they use the same code path. getRefreshedLocation

Member Author

alexjo2144 Apr 25, 2022

Updated to include an invalidateCaches parameter, which is set to true when refresh is called, and false when current is called. That meets the expectations of the Iceberg library

findepi reviewed

View reviewed changes

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java Outdated Show resolved Hide resolved

findepi approved these changes

View reviewed changes

alexjo2144 and others added 2 commits

April 25, 2022 11:20


          Support row level deletes in the Iceberg connector

411c969

Support removing individual rows by writing positional delete files
compatible with v2 of the Iceberg specification.

Co-authored-by: Jack Ye <yzhaoqin@amazon.com>


          Improve Iceberg commit to HMS

af51789

The Iceberg library has a built in commit retry, but in
order to use it the TableOperations must throw the right
kind of exception, and
AbstractMetastoreTableOperations#getRefreshedLocation
must refresh the metastore cache.

alexjo2144 force-pushed the iceberg/row-level-deletes-write branch from d83a9d4 to af51789 Compare

April 25, 2022 15:21

findepi merged commit cc017e2 into trinodb:master

findepi mentioned this pull request

Release notes for 379 #12088

Closed

github-actions bot added this to the 379 milestone

mosabua mentioned this pull request

Add Trino 379 release notes #12106

Merged

alexjo2144 deleted the iceberg/row-level-deletes-write branch

May 10, 2022 17:49

This was referenced May 13, 2022

Add missing Iceberg release note to 379 #12384

Merged

Iceberg HMS Catalog does not retry failing commits #12419

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed docs