refactor(iceberg): replace reflection-based positional delta writer - #1118
Draft
shubham19may wants to merge 22 commits into
Draft
refactor(iceberg): replace reflection-based positional delta writer#1118shubham19may wants to merge 22 commits into
shubham19may wants to merge 22 commits into
Conversation
…e so for reference it is
…emitting pos deltes
BaseDeltaTaskWriter reached into two private fields of Iceberg's BaseEqualityDeltaWriter to reach its data and pos-delete writers. That fails at runtime in a writer constructor, so an Iceberg field rename would break every sync with no static signal. Positional writes now go through PositionalDeltaWriter, which extends BaseTaskWriter and owns both halves directly: a RollingFileWriter per partition for data, and a SortingPositionOnlyDeleteWriter per partition for deletes. Everything it touches is public or inherited-protected. A partition key is computed for every row, so an unpartitioned table is one map entry on the empty struct instead of a separate writer class. BaseDeltaTaskWriter goes back to being the equality-only writer it was before this branch; it is now identical to staging apart from javadoc. Dropping BaseEqualityDeltaWriter also drops its insertedRowMap, which the legacy path relies on: the row index only learns positions once a batch is answered, so within one batch every update of a key carries the same superseded location and the second would otherwise leave both rows live. PositionalDeltaWriter reimplements it keyed on the identifier values rather than a copied StructLike, and scopes it to a single batch: once the write runs are returned the caller addresses those rows itself, so holding them until commit would grow the map to every row written beforehand, most of which - a backfill writes each key once - can never be superseded. Each entry also holds its partition's delete writer directly instead of a copied PartitionKey, so writing a row allocates nothing extra. Also in this change: - Write-run tracking is gated on positional mode. Equality-mode users were paying two extra partition-key evaluations per record and receiving a WriteRun response the caller discards, which on an interleaved partitioned table is one run per record. - The GET_OR_CREATE_TABLE equality-delete probe, a full planFiles() scan of the table per writer thread per sync, is gated the same way. - PositionTrackableWriter gains willWrite(), so a row the writer skips no longer gets a run entry pointing at the position the next row takes, and batchCompleted(), which lets a writer release per-batch state. - Cancelling mid-batch returns no runs, since the writer is discarded. - RowDelta now asks Iceberg to validate what positional deletes depend on: validateFromSnapshot, validateDeletedFiles and validateDataFilesExist over the referenced data files. assertRowIndexCurrent only covers a rewrite that lands before the pre-check; one landing between the refresh and the catalog commit previously produced deletes that resolved to nothing. Equality mode is left alone: its deletes match on key, and the positional deletes it occasionally emits address rows written in the same commit. Verified against a real table on a local Hadoop catalog, and end to end on Postgres to Iceberg covering partitioned fan-out, a chunked backfill over four chunks, a 12k-record CDC batch repeating 3000 keys, cursor-based incremental, and an equality-mode regression run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
shubham19may
force-pushed
the
feat/interoperability-temp
branch
from
August 15, 2026 15:52
c548cda to
c539df6
Compare
…index BatchEmitter handed every batch to onNext regardless of whether the transport could take it. gRPC buffers what it is given, so a scan that outruns its caller grew that buffer for the length of the table - and the caller is doing a map insert and an index lookup per entry, so outrunning it is the normal case. On a large table both ends could end up holding the whole thing. Sends now wait for ServerCallStreamObserver.isReady(), woken by an onReady handler, which lets the slower side set the pace and keeps this server at flat memory. The wait is deliberately unbounded: a caller that is merely slow should throttle the scan rather than fail it. A wait that never resolves would be invisible, so it is reported every 30 seconds. The same wait notices a caller that has gone away. Until now nothing did: the record path polls grpcContext.isCancelled() per record, but a scan whose client disconnected would read the entire table for nobody, holding an executor thread and an Iceberg iterator the whole time. Cancellation unwinds the scan and is reported without touching the response, since the call is already closed and sending a status would only throw. Falls back to plain sends when the observer is not a ServerCallStreamObserver, so a caller holding a bare observer still works, just without back pressure. Verified end to end: wiping the local index forces a full rebuild, which streamed 50000 entries as five batches through the flow-controlled path with no stall reports, and the table came back 50000 rows / 50000 distinct ids with the 5000 updated rows superseded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces the reflection-based positional delta writer, and fixes the regressions equality-mode users would otherwise have picked up from this branch.
Why
BaseDeltaTaskWriterreached into two private fields of Iceberg'sBaseEqualityDeltaWriter:That fails at runtime, in a writer constructor, on the first record of a sync — so an Iceberg field rename breaks every sync with no static signal. The reflection was forced by the base class:
BaseEqualityDeltaWriterexposes onlywrite/delete/deleteKey/close, with no way to emit an arbitrary positional delete. In positional mode we were extending a class whose entire purpose — equality deletes andinsertedRowMap— we had bypassed.What changed
PositionalDeltaWriter(new) extendsBaseTaskWriterand owns both halves directly: aRollingFileWriterper partition for data, and aSortingPositionOnlyDeleteWriterper partition for deletes. Everything it touches is public or inherited-protected. A partition key is computed for every row, so an unpartitioned table is one map entry on the empty struct — no partitioned/unpartitioned writer split.BaseDeltaTaskWritergoes back to being the equality-only writer it was before this branch. It is now identical tostagingapart from javadoc, so equality mode is provably untouched.insertedRows. DroppingBaseEqualityDeltaWriteralso drops itsinsertedRowMap, which the legacy path depends on. The row index only learns positions once a batch is answered, so within a batch every update of a key carries the same superseded location and the second would leave both rows live. Reimplemented keyed on the identifier values rather than a copiedStructLike, and scoped to a single batch via a newbatchCompleted()hook — holding it until commit would grow the map to every row written beforehand, most of which (a backfill writes each key once) can never be superseded.Also fixed
WriteRunresponse the caller discards — on an interleaved partitioned table that is one run per record, so a 10k batch shipped megabytes that were thrown away.GET_OR_CREATE_TABLEequality-delete probe — a fullplanFiles()scan of the table, per writer thread, per sync — is gated the same way. It was unconditional.willWrite()added toPositionTrackableWriter: a row the writer skips no longer gets a run entry pointing at the position the next row takes. Unreachable today becausekeepDeletesis hardcoded true, but the run encoding could not represent "not written" and the corruption would have been silent.RowDeltanow asks Iceberg to validate what positional deletes depend on:validateFromSnapshot,validateDeletedFiles, andvalidateDataFilesExistover the referenced data files.assertRowIndexCurrentonly covers a rewrite landing before the pre-check; one landing between the refresh and the catalog commit previously produced deletes that resolved to nothing — silent duplicates rather than a failed commit. Equality mode is deliberately left alone: its deletes match on key, and the positional deletes it occasionally emits address rows written in the same commit, so nothing concurrent can invalidate them.Testing
Unit — 19 tests
Against a real Iceberg table on a local Hadoop catalog. Rows are committed through
RowDeltaand read back with a normal scan, so Iceberg decides whether each delete applied rather than the test asserting on writer internals.addToTablePerSchemaloop over two batches on one uncommitted writer sessionvalidationRefusesDeletesWhoseDataFileWasRewritten— commits a rewrite, assertsValidationExceptionEQUALITY_DELETESand does not use the new writerWriteRunshape: contiguous writes collapse to one run; interleaved partitions still map every record to exactly one(path, position)End to end — Postgres → Iceberg,
--delete-type pos/{region, identity})(path, pos)of the superseded rowr → v2/v3 → v4, one live row--delete-type eq: equality deletes still produced, no row-index directory created for the streamNotes for reviewers
Two findings from my earlier review were wrong and are retracted here, both verified in the files:
IcebergUtilsetswrite.metadata.metrics.column.file_path=full, which withDeleteGranularity.FILEmakes deletes file-scoped, so Iceberg matches them to data files by path, not partition. Confirmed in e2e: a delete file physically inregion=ustargeting aregion=indata file was applied.DeleteGranularitytoPARTITIONis not the easy lever it looks like. A partition-granularity file spans many data files, itsfile_pathbounds widen, it stops being file-scoped, and partition-changing updates silently break. FILE granularity is load-bearing for correctness. (The file-count cost turned out modest in practice — 12k updates over 3000 keys produced 2 delete files.)Method references to
RollingFileWriterdo not link —close()is inherited from the package-privateBaseRollingWriter, soRollingFileWriter::closedies at runtime withLambdaConversionException.close()uses plain loops with suppressed-exception accumulation.Still open, both Go-side and out of scope here:
deleteFilePath, and Go'sNeedsRowIndexdoes not check identifier fields orupsert— sono_identifier_fields+possilently produces duplicates. Wants a config-validation refusal.opType != "r"skips the index lookup, so a backfill re-run into a non-empty table produces duplicates where equality mode produced none. One line in each writer.Unrelated, found while testing:
protocol/sync.goloads state only when--stateis passed. Without it the file is written but never read back, soPreCDCcallsAdvanceLSNon every run and CDC reads zero records. The e2e procedure in.cursor/rules/olake.mdc§6 omits--state, which makes CDC untestable as written.Tests are not in this branch —
.gitignorehas a baretestpattern that matches any directory namedtest, includingsrc/test, so the Java test tree is ignored repo-wide. Worth narrowing separately.