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 unused bounds data from Iceberg DeleteFiles #22688

Merged
merged 1 commit into from
Jul 18, 2024
Merged

Remove unused bounds data from Iceberg DeleteFiles #22688

merged 1 commit into from
Jul 18, 2024

Conversation

alexjo2144
Copy link
Member

Description

DeleteFile is sent to workers so they are data size sensitive.

Additional context and related issues

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Jul 16, 2024
@github-actions github-actions bot added the iceberg Iceberg connector label Jul 16, 2024
@raunaqmorarka
Copy link
Member

PTAL at test failures

@findinpath findinpath requested a review from ebyhr July 17, 2024 06:52
@findinpath
Copy link
Contributor

https://github.com/trinodb/trino/actions/runs/9962214866/job/27526300884?pr=22688

tests               | 2024-07-17 00:40:40 INFO: FAILURE     /    io.trino.tests.product.iceberg.TestIcebergSparkCompatibility.testCleaningUpIcebergTableWithRowLevelDeletes [PARQUET, AVRO] (Groups: profile_specific_tests, iceberg) took 2.1 seconds
tests               | 2024-07-17 00:40:40 SEVERE: Failure cause:
tests               | io.trino.tempto.query.QueryExecutionException: java.sql.SQLException: Query failed (#20240716_185540_00200_uee7x): Cannot invoke "java.util.Map.get(Object)" because the return value of "org.apache.iceberg.DeleteFile.lowerBounds()" is null
tests               | 	at io.trino.tempto.query.JdbcQueryExecutor.execute(JdbcQueryExecutor.java:119)
tests               | 	at io.trino.tempto.query.JdbcQueryExecutor.executeQuery(JdbcQueryExecutor.java:84)
tests               | 	at io.trino.tests.product.utils.QueryExecutors$1.lambda$executeQuery$0(QueryExecutors.java:54)
tests               | 	at dev.failsafe.Functions.lambda$toCtxSupplier$11(Functions.java:243)
tests               | 	at dev.failsafe.Functions.lambda$get$0(Functions.java:46)
tests               | 	at dev.failsafe.internal.RetryPolicyExecutor.lambda$apply$0(RetryPolicyExecutor.java:74)
tests               | 	at dev.failsafe.SyncExecutionImpl.executeSync(SyncExecutionImpl.java:187)
tests               | 	at dev.failsafe.FailsafeExecutor.call(FailsafeExecutor.java:376)
tests               | 	at dev.failsafe.FailsafeExecutor.get(FailsafeExecutor.java:112)
tests               | 	at io.trino.tests.product.utils.QueryExecutors$1.executeQuery(QueryExecutors.java:54)
tests               | 	at io.trino.tests.product.iceberg.TestIcebergSparkCompatibility.testCleaningUpIcebergTableWithRowLevelDeletes(TestIcebergSparkCompatibility.java:2280)

@alexjo2144 alexjo2144 force-pushed the ajo/reduce-delete-file-size branch 2 times, most recently from 128631a to 75d65dd Compare July 17, 2024 15:47
raunaqmorarka
raunaqmorarka reviewed Jul 17, 2024
View reviewed changes
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/delete/DeleteFile.java
Map<Integer, byte[]> upperBounds = firstNonNull(deleteFile.upperBounds(), ImmutableMap.<Integer, ByteBuffer>of())
.entrySet().stream().collect(toImmutableMap(Map.Entry::getKey, entry -> getWrappedBytes(entry.getValue()).clone()));
Optional<Long> rowPositionLowerBound = Optional.ofNullable(deleteFile.lowerBounds())
.map(bounds -> bounds.get(DELETE_FILE_POS.fieldId()))
Copy link
Member

Choose a reason for hiding this comment

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

do we need to filter out nulls here ?
i'm not sure that having bounds for DELETE_FILE_POS field is always guaranteed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, better safe than sorry. Don't want to end up with an Optional.of(null) value

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I was wrong about that, if bounds doesn't have an entry for the DELETE_FILE_POS column it'll return null. Optional.map converts null returns to empty

@raunaqmorarka raunaqmorarka merged commit aff587e into trinodb:master Jul 18, 2024
42 of 43 checks passed
@github-actions github-actions bot added this to the 453 milestone Jul 18, 2024
@mosabua mosabua mentioned this pull request Jul 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raunaqmorarka raunaqmorarka raunaqmorarka approved these changes

@wendigo wendigo wendigo approved these changes

@ebyhr ebyhr Awaiting requested review from ebyhr

Assignees
No one assigned
Labels
cla-signed iceberg Iceberg connector
Milestone
453
Development

Successfully merging this pull request may close these issues.
4 participants
@alexjo2144 @raunaqmorarka @findinpath @wendigo