Reduce FileSystem calls in LinePageSourceFactory #18959
Merged
Conversation
This file contains 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
electrum
approved these changes
Sep 9, 2023
1a7ef1f to
23206e3
Compare
|
/test-with-secrets sha=cd6fc9fe6f4669af151c5fe0a88f2ac8480b392d |
|
The CI workflow run with tests that require additional secrets has been started: https://github.com/trinodb/trino/actions/runs/6174060858 |
|
@electrum - one more commit was added to fix the related |
Avoids calling exists() and length() on TrinoInputFile inside of LinePageSourceFactory as part of LinePageSource creation. Both of which will map to some API metadata lookups like S3 HeadObject which is unnecessary since: 1. GetObject will throw FileNotFoundException anyway if the file does not exist and issuing a HeadObject eagerly doesn't guarantee the object will still exist for a subsequent GetObject call 2. Adjusting the split length based on the exact file size doesn't prevent data corruption, it's only meant to handle the scenario where encrypted objects may include the last block padding bytes into the file size. This can be detected by reading until an EOF is encountered and will similarly not be prevented by checking the length eagerly since the size could still change between the two calls.
cd6fc9f to
b9fdcd9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Avoids calling
exists()andlength()onTrinoInputFileinside ofLinePageSourceFactory, both of which will map to some API metadata lookups like S3's HeadObject which is unnecessary since:FileNotFoundExceptionanyway if the file does not exist and issuing aHeadObjecteagerly doesn't guarantee the object will still exist for a subsequentGetObjectcallRelease notes
( ) 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.
(x) Release notes are required, with the following suggested text: