vladislav-sidorovich commented on PR #15407: URL: https://github.com/apache/iceberg/pull/15407#issuecomment-4833023765
> Thanks for sticking with the review iterations. I went back through the existing threads before adding anything here, so I’m not reopening what’s already settled. > > ..... > > Once the correctness issues are fixed and the follow-ups are tracked, happy to take another pass and approve. @laskoviymishka Thank you for the review and the summary comment. 1. >`The DV `OutputFileFactory` is fixed at `(1, 1)`...` The following sequence of action ``` 1. Add(file_A, DV_1) 2. Remove(file_A, DV_1) 3. Add(file_A, DV_2) ``` is possible in multiple Delta commits and handled in `io.delta.kernel.internal.replay.ActiveAddFilesIterator` by `UniqueFileActionTuple`. At the same time this sequence is impossible in the scope of single Delta snapshot and handled by `DMLWithDeletionVectorsHelper.scala` for writes, and `ConflictChecker.scala` for reads (see `lazy val addedFilePathToActionMap: Map[String, AddFile]`). The conversion works snapshot by snapshot, so it's correct. The factory is not closable, but `DVFileWriter dvWriter` is closed in try-with-resource section in `convertDeltaDVsToIcebergDVs`. 2. > execute() returns a method-reference lambda instead ... Done. 3. > icebergPropertiesBuilder is an instance ImmutableMap.Builder ... throw on duplicate keys is an expected behavior. It’s never happened in a proper execution but will throw an exception for invalid usage. This strategy also aligns with your previous comments regarding fail with explicit message vs silent corruption. 4. > The golden-table test only checks that execute() doesn’t throw .... It's actually an interesting topic to discus. My ideas was the following: use unit-tests `TestBaseSnapshotDeltaLakeKernelTableAction` to verify conversion logic like supported / not supported features, valid/invalid parameters, basically logic I wrote for the conversion. Testing the table data is done in the integration tests and moving it to unit-tests will make them much slower. So, I see the following options: 1. Keep golden tables in unit-tests and follow the pattern to throw an exception instead of silent corruption. 2. Move golden tables to integration-tests and verify data What do you think? Other topics: 1. VACUUM - I'm not 100% aligned there. I believe VACUUM logic is covered and work properly. Please let me know if I missed anything. 1. `testConversionAfterVacuum` - tests to processes `VACUUM` and logs compaction. VACUUM deletes data files, and log compaction deletes logs before check points. So, in this test we have commits pointing to deleted files but it works properly because conversion support VACUUM and starts from the latest checkpoint. 1. Let me go again through all the comments to double check that I didn't miss anything. 1. I will move the implementation doc to issues as was requested. 1. I will move all the usage of internal Delta API to helper classes to have it in 1 place because it was highlighted by at least 2 reviewers . -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
