Core, Data, Spark, Flink: Remove of position delete files with row data - #17706
Core, Data, Spark, Flink: Remove of position delete files with row data#17706dramaticlly wants to merge 1 commit into
Conversation
Position deletes that carry row data were deprecated in 1.11.0 for removal in 1.12.0, and the spec documents them as unsupported. Following the approach prototyped in apache#13870, the writers reject row data rather than silently dropping it. Writes are rejected; reads are preserved for backward compatibility with delete files produced by earlier versions. Core, ORC, Parquet: - PositionDelete: removed set(CharSequence, long, R) and row(). The row field, size()==3 and the positional get/set are retained so delete files that already carry row data still read back; get(2) is null for anything written from now on. - Avro, ORC and Parquet buildPositionWriter() throw UnsupportedOperationException when a rowSchema is set. rowSchema itself stays on all three DeleteWriteBuilders, since equality deletes require it. - Avro.PositionAndRowDatumWriter is deleted, being unreachable. The PositionDeleteStructWriter in GenericOrcWriters and ParquetValueWriters no longer exposes the row column, and ORC's row-not-null precondition is dropped because it can no longer fire. - ORC and Parquet transformPaths is removed; its only reader was the row-data branch. The path+pos branch always hardcoded Function.identity(), so path handling is unchanged. - BasePositionDeltaWriter and BaseTaskWriter use set(path, pos). - RewriteTablePathUtil.PositionDeleteReaderWriter: the rowSchema overload is removed and the 4-arg writer() is now abstract. Rewritten position delete files no longer reproduce the row column. - PositionDeleteWriter's constructor is narrowed from FileAppender<? extends StructLike> to FileAppender<PositionDelete<T>>, as its 1.11.0 deprecation note scheduled for 1.12.0. Dropping the wildcard removes the unchecked cast; every caller already builds an appender of the exact type, so no call site changes. Erasure is unchanged, so this is a source-only break and revapi reports nothing. - PositionDeltaWriter.delete(CharSequence, long, T, PartitionSpec, StructLike) now accepts and ignores its row argument, so it is deprecated in favour of delete(CharSequence, long, PartitionSpec, StructLike) with removal scheduled for 1.13.0. The four-argument form is currently a default that delegates to the five-argument one; in 1.13.0 that flips, so the four-argument form becomes the method implementations override. BasePositionDeltaWriter is the only implementation and its override is marked accordingly. Data and engines: - GenericFileWriterFactory: both deprecated constructors and Builder.positionDeleteRowSchema are removed, along with the newPositionDeleteWriter override that existed only to honour the row schema. The surviving constructor keeps writerProperties, which Builder.build() sets and which has callers in Flink RowDataTaskWriterFactory and kafka-connect RecordUtils. Also removed 9 vestigial configure* overrides that override nothing now that BaseFileWriterFactory is gone. - SparkFileWriterFactory and SparkPositionDeltaWrite (all versions): deprecated constructors and row-schema wiring removed. - SparkPositionDeletesRewrite: DeleteWriter collapses from two writer factories to one, since incoming rows are no longer propagated. - RewriteTablePathSparkAction: SparkPositionDeleteReaderWriter implements the 4-arg writer(). - FlinkAppenderFactory is removed outright rather than updated, in all three Flink versions. It was deprecated in 1.11.0 in favour of FlinkFileWriterFactory, with class-level javadoc scheduling removal for 1.12.0, and its only references were the class itself and TestFlinkAppenderFactory. Tests: - The position_deletes metadata table keeps its row column so pre-1.12 delete files still read, but nothing can populate it, so tests that write rows and assert on them are dropped: TestPositionDeletesTable testSelect, testPartitionedTable, testArrayColumnFilter, testSchemaEvolutionAdd/Remove and testWriteSchemaEvolutionAdd/Remove. Each already declared the dependency with assumeThat(formatVersion) ... "DVs don't have row info in PositionDeletesTable". - Dropped TestAppenderFactory.testPosDeleteWriterWithRowSchema, TestFileWriterFactory.testPositionDeleteWriterWithRow and TestRewriteTablePathsAction.testPositionDeleteWithRow, plus the @disabled overrides that already anticipated their removal. - The Avro, ORC and Parquet row-data writer tests are replaced by ones asserting the new rejection. - TestWriterMetrics keeps checkRowStatistics unchanged; it already asserts the bounds contain no row fields, which is now the only possible outcome. - SimpleDataUtil.writeEqDeleteFile/writePosDeleteFile's FileAppenderFactory overloads go with FlinkAppenderFactory, which was the only FileAppenderFactory<RowData> implementation, so no Flink test can supply an argument to them. Neither had callers left: TestFlinkManifest, TestIcebergCommitter and TestIcebergFilesCommitter all pass a FileWriterFactory and resolve to the other overload, and TestIcebergFilesCommitter.writePosDeleteFile, the only caller of the position-delete overload, was itself already uncalled. revapi.yml records justifications for the resulting binary breaks.
d39351b to
d48f943
Compare
There was a problem hiding this comment.
Nit: flink/{v1.20,v2.0,v2.1}/FlinkAppenderFactory.java is removed entirely, but the PR body describes it as modified ("newPosDeleteWriter now builds path+pos writers"). The deletion is legitimate, the class is @deprecated since 1.11.0, will be removed in favor of FlinkFileWriterFactory, and is unreferenced in main, but its full removal spans a broader migration than PDWR removal alone - please confirm the scope and possibly correct the PR body description accordingly.
| * @deprecated since 1.12.0, will be removed in 1.13.0; the {@code row} is ignored, see {@link | ||
| * PositionDeltaWriter#delete(CharSequence, long, Object, PartitionSpec, StructLike)}. |
There was a problem hiding this comment.
Should this be without the Object?
| * @deprecated since 1.12.0, will be removed in 1.13.0; the {@code row} is ignored, see {@link | |
| * PositionDeltaWriter#delete(CharSequence, long, Object, PartitionSpec, StructLike)}. | |
| * @deprecated since 1.12.0, will be removed in 1.13.0; the {@code row} is ignored, see {@link | |
| * PositionDeltaWriter#delete(CharSequence, long, PartitionSpec, StructLike)}. |
| .writeProperties(writeProperties) | ||
| .build(); | ||
| SparkFileWriterFactory writerFactoryWithoutRow = | ||
| SparkFileWriterFactory writerFactory = |
There was a problem hiding this comment.
Could we add to the class javadoc, that if there is row-data in the position deletes then it will be removed after the rewrite?
There was a problem hiding this comment.
Same for the other Spark versions
|
Convert to draft for now, as current change in rewrite-position-delete and rewrite-table-path direct drop the row for existing position delete with row data, need to think of a better way to handle the writer deletion while not cause trouble for existing workload. Maybe both need to fail early if detected existing PDWR |
Follow up removal of #14045, this removes ability of writing Position Deletes With Row (PDWR) in java reference implementation. https://iceberg.apache.org/spec/#position-delete-files-with-row-data
After the change, the Java implementation will no longer write PDWR delete files. It will only support reading the position and file name columns from previously created delete files or those generated by other engines.
Core
PositionDelete.set(CharSequence, long, R)and getter ofrow()removed while keep thesize() == 3and the positionalget/setso that delete files already containing row data still read back correctly.Avro,ORCandParquetbuildPositionWriter()now throwUnsupportedOperationExceptionwhen arowSchemais set.rowSchemaitself stays on all threeDeleteWriteBuilders as it is still required for equality deletesAvro.PositionAndRowDatumWriterdeleted; thePositionDeleteStructWriters inGenericOrcWritersandParquetValueWritersno longer expose the row column, and ORC's row-not-null precondition is droppedORC/ParquettransformPathsremoved. Its only reader was the row-data branchRewriteTablePathUtil.PositionDeleteReaderWriter: therowSchemaoverload is gone and the 4-argwriter()is now abstract.Engines specific
GenericFileWriterFactory: both deprecated constructors andBuilder.positionDeleteRowSchemaremoved, along with thenewPositionDeleteWriteroverride that existed only to honour the row schema. Also removed 9 vestigialconfigure*overrides which override nothing now thatBaseFileWriterFactoryis gone (aFormatModelRegistryleftover rather than a position-delete concern, but in the same file).SparkFileWriterFactory×3,SparkPositionDeltaWrite×3,FlinkAppenderFactory×3: deprecated constructors and row-schema wiring removed;FlinkAppenderFactory.newPosDeleteWriternow builds path+pos writers.SparkPositionDeletesRewrite×3:DeleteWritercollapses from two writer factories to one.RewriteTablePathSparkAction×3:SparkPositionDeleteReaderWriterimplements the 4-argwriter().Behavior changes worth a close look
rowstay, writes are gone. Theposition_deletesmetadata table still has arowcolumn for backward compatibility with existing v2 delete files; it is null for anything written from now on.RewriteTablePathUtilpreviously detected row data in a source delete file and reproduced it; it can no longer do so, so rewritten position delete files contain path+pos only. Same for therewrite_position_delete_filespath inSparkPositionDeletesRewrite, which no longer propagates incoming rows.position_deletesmetadata table and testsThe table keeps exposing its
rowcolumn so that delete files written before 1.12.0 still read back. Nothing can populate it any more, so it reads as null for anything written by this version.Related test which writes PDWR gets dropped
Verification
gradlew check.palantir/revapi.ymlgains 16 entries across core, data, orc and parquetExtra Callout
d39351b include change for
PositionDeltaWriter.delete(CharSequence, long, T, PartitionSpec, StructLike)still takes arowparameter that is now ignored byBasePositionDeltaWriter. It is not deprecated, so narrowing it belongs in a 1.13.0 deprecation cycle.AI Disclosure
Model: Claude Opus 5 (1M context)
Platform/Tool: Claude Code
Human Oversight: reviewed
Prompt Summary: split #16449 into smaller self-contained PRs; verify each group compiles and tests green standalone
@pvary if you can help take a look at the change?
CC @szehon-ho for changes relate to position delete metadata table and PDWR in rewrite table path changes