aiborodin commented on PR #14503: URL: https://github.com/apache/iceberg/pull/14503#issuecomment-3488522797
> @danielcweeks @bryanck @amogh-jahagirdar There's an existing PR, which is currently being reviewed by @pvary, to add this functionality: https://github.com/apache/iceberg/pull/14445. > > My main concern with this change is that it doesn't fit into the existing validation framework in RowDelta/ReplacePartitions interfaces. @danielcweeks @bryanck @amogh-jahagirdar Merging this change will result in two places in code where validation occurs. In my opinion, and I think @pvary would agree, we should align with the existing validation API in [SnapshotProducer](https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/SnapshotProducer.java#L241) and corresponding interfaces in `RowDelta`, `ReplacePartitions` etc. to provide a single validation API, which will be clear and more maintainable in the future. For example, `RowDelta` already has the following validations: ```java RowDelta validateDataFilesExist(Iterable<? extends CharSequence> referencedFiles); RowDelta validateDeletedFiles(); RowDelta conflictDetectionFilter(Expression conflictDetectionFilter); ``` https://github.com/apache/iceberg/pull/14445 addresses these concerns and aligns with the existing validation APIs. I would appreciate it if we could continue reviewing https://github.com/apache/iceberg/pull/14445. cc: @pvary -- 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]
