pvary commented on PR #16449: URL: https://github.com/apache/iceberg/pull/16449#issuecomment-4798958445
> ⚠️ PositionDelete.set(path, pos, row) and row() — Kept without @Deprecated. The public deprecation notice said position deletes with row data are no longer supported, but internal code (BaseTaskWriter, BasePositionDeltaWriter, RewriteTablePathUtil, Avro) still relies on these methods. Reviewer: confirm the methods are safe to retain as package-internal helpers, or whether a follow-up should narrow visibility / push removal to 1.13.0. We should remove them. Some of the calls are already with `null`, so that should not be an issue, the others are not something where we can communicate the deprecation with annotation. That is why we added https://iceberg.apache.org/spec/#position-delete-files-with-row-data. So I suggest that remove the functionality too - Definitely in another PR. > BaseParquetReaders.createStructReader(List, StructType, Integer) — Changed from a concrete method (that delegated to the removed 2-param version) to abstract. Both existing subclasses (InternalReader, GenericParquetReaders) already override it. This is ok. > RewriteTablePathUtil.PositionDeleteReaderWriter.writer() — The 4-arg method was a default delegating to the removed 5-arg version. Made it abstract; updated all implementors (RewriteTablePathSparkAction in v3.5/v4.0/v4.1) to implement the 4-arg signature directly. This is ok. > MetricsConfig.forPositionDelete(Table) — All callers (FlinkAppenderFactory, RegistryBasedFileWriterFactory) updated to use the no-arg forPositionDelete() since row-level position delete metrics are no longer supported. This is ok. > GenericFileWriterFactory — Re-added writerProperties to the constructor and Builder since it was a public non-deprecated setter used by GenericAppenderHelper and kafka-connect RecordUtils. Reviewer: confirm this re-addition matches the intended public surface (i.e., the field was incidentally lost while removing GenericAppenderFactory and should remain on GenericFileWriterFactory). I don't see this "re-addition" in the PR, but it should be fine, as long as the `positionDeleteRowSchema` is not added back -- 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]
