Include partition key in the Iceberg data file location #25169
+15
−2
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
mmaslankaprv
force-pushed
the
dl-paritition-key-location
branch
from
0ccb8df to
ee8d61d
Compare
dotnwat
reviewed
Feb 25, 2025
mmaslankaprv
force-pushed
the
dl-paritition-key-location
branch
2 times, most recently
from
9d2f03f to
030044a
Compare
Retry command for Build#62390please wait until all jobs are finished before running the slash command |
CI test resultstest results on build#62390
test results on build#62405
|
Including a partition key derived path component in the Iceberg data files remote location. Signed-off-by: Michał Maślanka <[email protected]>
mmaslankaprv
force-pushed
the
dl-paritition-key-location
branch
from
030044a to
bb37c49
Compare
mmaslankaprv
requested review from
dotnwat,
nvartolomei,
oleiman and
andrwng
Retry command for Build#62405please wait until all jobs are finished before running the slash command |
andrwng
approved these changes
Feb 27, 2025
nvartolomei
reviewed
Feb 28, 2025
There was a problem hiding this comment.
In addition to what Andrew said, you can try mixing together Datalake3rdPartyMaintenanceTest with DatalakeE2ETests.test_iceberg_files_location to ensure that the paths are as expected and remain so post rewrite by 3rd party engine. I.e. ensure that our interpretation of paths matches 3rd party engines*.
* This would be best effort anyway. 3rd party query engines can be configured not to use partition key in the path, or they must even custom implementations and it is not practical to try and achieve 100% compatibility with all of them. At most, we can try to be consistent with one of them.
| @@ -36,7 +36,7 @@ ss::future<checked<remote_path, translation_task::errc>> execute_single_upload( | |||
| retry_chain_node& parent_rcn, | |||
| lazy_abort_source& lazy_as) { | |||
| auto file_remote_path = remote_path{ | |||
| file.table_location / remote_path_prefix | |||
There was a problem hiding this comment.
Should we remove remote_path_prefix completely with this commit?
The prefix contains n/t/p and we were discussing removing it next time we touch this code and introduce partition key path. Once we run an optimize task with say trino/spark, they will rewrite the paths and remove that information.
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.
Including a partition key derived path component in the Iceberg data files remote location. The partition key being part of the path allow users to better navigate the Iceberg data files in the object store. Additionally it will allow per partition access control.
Backports Required
Release Notes