(IcebergIO) bugfix: wire writeProperties through table create request, not DataWriteBuilder - #39645
Conversation
…, not DataWriteBuilder
|
cc @ahmedabu98 - I know it's last minute, but would it be possible to get this into the 2.76 release? 🙏 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39645 +/- ##
============================================
- Coverage 58.15% 58.15% -0.01%
- Complexity 13085 13089 +4
============================================
Files 2521 2521
Lines 264850 264862 +12
Branches 10788 10788
============================================
+ Hits 154023 154025 +2
- Misses 105051 105066 +15
+ Partials 5776 5771 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ahmedabu98
left a comment
There was a problem hiding this comment.
This PR merges writeProperties and tableProperties into essentially the same thing. I think we should keep them separate.
table_propertiesgets applied to a table on creation timewrite_propertiesapplies only to the current execution and takes precedence overtableProperties, but does not actually mutate the table's properties.
This is the behavior I'm seeing with other engines like Flink and Spark as well.
For the issue you're running into, I think you can just supply table properties to the connection. It's not currently exposed in the raw IcebergIO API, but it's in Managed(ICEBERG)
| if (writeProperties != null && !writeProperties.isEmpty()) { | ||
| parquetBuilder.setAll(writeProperties); | ||
| } | ||
| icebergDataWriter = parquetBuilder.build(); |
There was a problem hiding this comment.
Removing this makes writeProperties a no-op if the table already exists
| if (writeProperties != null) { | ||
| tableProperties.putAll(writeProperties); | ||
| } |
There was a problem hiding this comment.
writeProperties are supposed to be execution-scoped. They're not meant to be persisted in the actual table's properties
|
Assigning reviewers: R: @Abacn for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
I discovered when testing a snapshot of #39250 that some properties, like bloom filter enabling, were getting propagated, but non-Parquet-related properties, like
write.data.path, were not.Root cause: writeProperties passed via
IcebergIO.writeRows().withWriteProperties(...)were only forwarded toParquet.DataWriteBuilder.setAll()in RecordWriter, but were never stored as Iceberg table properties. This means:It turns out that Iceberg's forTable(table) (https://github.com/apache/iceberg/blob/main/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java#L905-L911) already calls setAll(table.properties()), loading all table properties into the builder's config. So the correct place to apply writeProperties is on the table itself, not as a second setAll() on the builder.
This PR merges
writePropertiesinto the table properties at creation time in bothRecordWriterManager.loadOrCreateTable()andWritePartitionedRowsToFiles.loadOrCreateTable().Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.