Add support for writing deletion vectors in Delta Lake#22102
Merged
Conversation
ebyhr
force-pushed
the
ebi/delta-write-deletion-vector
branch
from
72af1af to
8ea6475
Compare
ebyhr
force-pushed
the
ebi/delta-write-deletion-vector
branch
3 times, most recently
from
a55bc53 to
7d52f10
Compare
ebyhr
force-pushed
the
ebi/delta-write-deletion-vector
branch
from
7d52f10 to
e3ce7cb
Compare
ebyhr
force-pushed
the
ebi/delta-write-deletion-vector
branch
from
e3ce7cb to
8f8cddc
Compare
ebyhr
force-pushed
the
ebi/delta-write-deletion-vector
branch
4 times, most recently
from
4152170 to
8ab2964
Compare
ebyhr
force-pushed
the
ebi/delta-write-deletion-vector
branch
4 times, most recently
from
5242022 to
42769ca
Compare
ebyhr
requested review from
alexjo2144,
findinpath,
mayankvadariya and
raunaqmorarka
wendigo
reviewed
Jul 25, 2024
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/delete/Base85Codec.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java
Outdated
Show resolved
Hide resolved
ebyhr
force-pushed
the
ebi/delta-write-deletion-vector
branch
2 times, most recently
from
37eaaa8 to
c9bc2da
Compare
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/delete/DeletionVectors.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java
Outdated
Show resolved
Hide resolved
ebyhr
force-pushed
the
ebi/delta-write-deletion-vector
branch
from
c9bc2da to
70ead35
Compare
ebyhr
force-pushed
the
ebi/delta-write-deletion-vector
branch
from
70ead35 to
b2de0c2
Compare
| deletedRows.or(deletion.rowsDeletedByDelete()); | ||
| deletedRows.or(deletion.rowsDeletedByUpdate()); | ||
|
|
||
| TrinoInputFile inputFile = fileSystem.newInputFile(Location.of(path.toStringUtf8())); |
Member
There was a problem hiding this comment.
Do we know size of deletion vector in advance from io.trino.plugin.deltalake.transactionlog.DeletionVectorEntry#sizeInBytes or any other metadata ?
If we do, then using it in newInputFile would probably save a FS call.
Member
Author
There was a problem hiding this comment.
This inputFile is a Parquet file, not deletion vector. I think that's possible, but it requires some refactoring. Let me handle in a follow-up.
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/delete/DeletionVectors.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/delete/DeletionVectors.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/delete/RoaringBitmapArray.java
Show resolved
Hide resolved
ebyhr
force-pushed
the
ebi/delta-write-deletion-vector
branch
2 times, most recently
from
6883a04 to
a1587b5
Compare
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMergeSink.java
Outdated
Show resolved
Hide resolved
plugin/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeMetadata.java
Outdated
Show resolved
Hide resolved
Member
There was a problem hiding this comment.
When we write a new deletion vector, is it mandatory as per the spec that it should be a union of all deleted/updated rows so far, or is that just how we've implemented it ?
Is cleanup of old deletion vectors something already handled by some optimize/vaccum procedure in Trino ?
Member
Author
There was a problem hiding this comment.
It's mandatory for compatiblity with Delta Lake. Otherwise, they return the wrong results if I remember correctly.
The cleanup should be handled in #22809
ebyhr
force-pushed
the
ebi/delta-write-deletion-vector
branch
from
a1587b5 to
37622e1
Compare
ebyhr
force-pushed
the
ebi/delta-write-deletion-vector
branch
from
37622e1 to
e9ebfcb
Compare
Member
Author
|
Addressed comments. |
raunaqmorarka
approved these changes
Aug 14, 2024
findinpath
reviewed
Aug 14, 2024
Comment on lines
+359
to
+361
| RoaringBitmapArray deletedRows = loadDeletionVector(Location.of(path.toStringUtf8())); | ||
| deletedRows.or(deletion.rowsDeletedByDelete()); | ||
| deletedRows.or(deletion.rowsDeletedByUpdate()); |
Contributor
There was a problem hiding this comment.
There was a discussion with @findepi about whether If the amount of rows deleted from file depasses a certain quota, we should consider to proactively rewrite the file.
Is there a potential follow-up here?
Member
Author
There was a problem hiding this comment.
It's a potential follow-up task. I don't expect we will handle it shortly though.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #17063
Release notes
(x) Release notes are required, with the following suggested text: