V4 enable broad tests#16938
Draft
stevenzwu wants to merge 13 commits into
Draft
Conversation
stevenzwu
force-pushed
the
v4_enable_broad_tests
branch
10 times, most recently
from
7e6ecb9 to
29ccd8e
Compare
stevenzwu
force-pushed
the
v4_enable_broad_tests
branch
from
29ccd8e to
e8feff4
Compare
stevenzwu
force-pushed
the
v4_enable_broad_tests
branch
11 times, most recently
from
18f0817 to
d2d7eb3
Compare
stevenzwu
force-pushed
the
v4_enable_broad_tests
branch
11 times, most recently
from
000d552 to
17aeaa9
Compare
Introduces the Phase 1 foundation for v4 metadata tree writes: TrackedFile and supporting types are extended with v4-specific shape, and the new ContentEntryAdapters utility projects legacy ManifestEntry rows and ManifestFile references into TrackedFile instances for the v4 content_entry schema. ManifestFile gains V4_FORMAT_VERSION and LEGACY_FORMAT_VERSION constants and default formatVersion() / recordCount() accessors. GenericManifestFile gets a v4 constructor variant that populates these fields at construction time; pre-v4 callers use the existing 17-arg constructor which delegates with defaults. ContentEntryAdapters.fromManifestFile dispatches the record_count source by the manifest's intrinsic formatVersion: v4+ manifests must carry a non-null recordCount populated by the writer or reader; pre-v4 manifests have it summed from the per-status file counts. TestContentEntryAdapters covers ADDED/EXISTING/DELETED tracking shape, the unpartitioned and partitioned-projection paths, format_version dispatch, record_count requirement for v4+, and the rejected status transitions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
stevenzwu
force-pushed
the
v4_enable_broad_tests
branch
5 times, most recently
from
82c42f7 to
b149982
Compare
…add matching reader V4Writer and V4DeleteWriter now emit content_entry Parquet rows via TrackedFileWrapper/ContentEntryAdapter rather than the legacy manifest_entry Avro shape. ContentEntryReader and ContentEntryManifestReaderAdapter project content_entry rows back to ManifestEntry<DataFile/DeleteFile> so all downstream consumers (ManifestGroup, MergingSnapshotProducer rewrite paths) work unchanged. Read-path dispatch in ManifestFiles is layered: 1. Avro manifests are always legacy (no file inspection). 2. Snapshot-tree callers thread an Integer writerFormatVersion hint through the new package-private read overloads: 1 routes to ContentEntryReader, 0 routes to legacy. 3. Callers without a hint (tests writing-then-reading, ad-hoc tooling) fall back to inspecting the Parquet footer schema for field id 134 (content_type) or 147 (tracking). The footer read is delegated to InternalParquet via DynMethods so core has no compile-time dependency on iceberg-parquet. Key design choices: - TrackedFile.schemaWithContentStats omits partition and content_stats when their struct types are empty (Parquet rejects empty groups). - TrackedFileWrapper uses hasPartition/hasContentStats flags to map positions dynamically when either optional group is absent. - V4Writer.add(DataFile) bypasses Delegates.suppressFirstRowId so per-entry firstRowId is stored in the tracking struct rather than at manifest level. - ContentEntryReader.setEntry uses wrapAppendPreservingFirstRowId for ADDED entries so firstRowId read from the tracking struct is not re-suppressed. - ContentEntryAdapter preserves firstRowId for EXISTING entries so uncommitted manifests can round-trip per-entry row IDs. - ContentEntryManifestReaderAdapter applies the same committed/uncommitted firstRowId nullification logic as ManifestReader.idAssigner. - ContentEntryManifestReaderAdapter.iterator tracks ordinal position and sets fileOrdinal and manifestLocation on each BaseFile to match Avro reader behavior. - Parquet.readSchema(InputFile) is a new public helper that returns just the Iceberg-converted file schema; InternalParquet.readSchema delegates to it for the DynMethods entry point. - v4 spec forbids content_type=POSITION_DELETES (PR apache#16025); three TestManifestReader tests that write standalone position-delete files / DV delete files are guarded with assumeThat isLessThan(4) and will be removed once PR apache#16677 (or its successor) gates v4 out of the broad parameterized test suite during incubation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Introduce the v4 root manifest write/read pair — the replacement for the manifest list in format version 4. RootManifestWriter emits content_entry Parquet rows (content_type=DATA_MANIFEST or DELETE_MANIFEST) with manifest_info counts from each ManifestFile. The writer accepts an explicit writer_format_version (0 for legacy v3 leaves carried over in a v3->v4 upgrade; 1 for v4 content_entry leaves) so Phase 5's SnapshotProducer can set it correctly per entry. RootManifestReader reconstructs GenericManifestFile objects from those rows. Direct data-file entries (the small-write optimization) are skipped with a DEBUG log; that path is deferred to a future phase. RootManifests is the static factory (analogous to ManifestLists) with write() and two read() overloads. TestRootManifest covers round-trips for data/delete manifests, key metadata, multiple manifests, legacy writer_format_version=0 entries, and the version guard on write(). The empty Parquet group limitation is resolved by using a single dummy optional boolean field (field id 99999/_unpartitioned) for the partition struct and a separate dummy (field id 99998/_no_stats) for content_stats; both are always null on write and ignored on read. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Phase 4 of the v4 metadata-tree write-path plan. Adds rootManifestLocation() to the Snapshot API, plumbs formatVersion and rootManifestLocation fields through BaseSnapshot with a constructor that enforces exactly one of manifest-list or root-manifest is set, updates SnapshotParser to read/write the new root-manifest JSON key, and dispatches cacheManifests() to RootManifests.read() for format version >= 4. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Wire SnapshotProducer.apply() to write a root manifest (.parquet) instead of a manifest list (.avro) when the table format version is >= 4. The v4 path uses RootManifests.write(), derives ADDED/EXISTING status per manifest from snapshotId comparison, and carries firstRowId + addedRows for row lineage. The v1-v3 path is unchanged. The commit() cleanup now resolves the committed location through both manifestListLocation() and rootManifestLocation() so it handles both v3 and v4 snapshots. RootManifestWriter gains a three-argument add() overload that accepts an explicit EntryStatus, needed to emit EXISTING for carried-over leaf manifests in multi-snapshot root manifests. TestV4SnapshotProducer covers: single append (root manifest .parquet, DATA_MANIFEST entry ADDED with writer_format_version=1), two appends (first leaf EXISTING, second ADDED), and delete-file (rewritten leaf ADDED, unchanged leaf EXISTING, deleted file DELETED in rewritten leaf). Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…airing Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Phase 7: project v4 colocated deletion vectors as DeleteFile entries for DeleteFileIndex. ContentEntryReader.colocatedDVDeleteFiles() walks a v4 leaf data manifest and yields one GenericDeleteFile(POSITION_DELETES, PUFFIN) per live row with an attached DV. ManifestGroup pulls these from each v4 data manifest and feeds them to DeleteFileIndex.Builder via a new addDeleteFiles() entry so they index by referencedDataFile() alongside delete-manifest contents. RootManifestWriter now substitutes ManifestWriter.UNASSIGNED_SEQ with the commit's sequence number when writing leaf-manifest references (mirroring V3Writer's wrapper), so InheritableMetadata correctly assigns the DV's effective dataSequenceNumber on read-back. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Targeted fixes so v4 writes parameterize cleanly across the SnapshotProducer subclass family (FastAppend, MergeAppend, RowDelta, RewriteFiles, RewriteManifests, ReplacePartitions, RemoveSnapshots) and v4 commits round-trip correctly: - ContentEntryAdapters: substitute a placeholder partition type (single optional `_unpartitioned` boolean, mirroring RootManifestWriter) when the writer's spec is unpartitioned. Parquet rejects empty group columns, so this is required for v4 leaf manifests on unpartitioned tables. Adapter also normalizes partition data: when the writer spec is unpartitioned, ignore any partition tuple carried by the file (which may have come from a different spec, e.g. a DataFile built with withPartitionPath then appended to an unpartitioned table) and emit an empty partition struct. Add `fromDataFile`/ `fromDeleteFile` overloads that accept the writer spec; existing two-arg overloads are preserved for tests. - Preserve a null entry snapshot_id round-trip through the v4 content_entry layout. The on-disk snapshot_id column is optional, so when the source entry has snapshot_id=null (the inheritance path used by FastAppend.appendManifest / MergeAppend), clear the placeholder in the resulting TrackedFile so the reader yields null and InheritableMetadata can populate the committing snapshot's id. Without this, downstream consumers saw snapshot_id=0 and validation against the expected snapshot id failed. - BaseSnapshot.cacheManifests: route the v4 root-manifest read through fileIO.newInputFile(ManifestListFile) so encryption-aware FileIOs decrypt the root manifest. Without this, encrypted v4 tables failed with ParquetCryptoRuntimeException on the first read after commit. - MergingSnapshotProducer.rewriteLeafManifestsWithDVs and BaseRewriteManifests' rewrite loop: use EncryptingFileIO.combine and pass the manifest's writer_format_version hint so the reader dispatches without inspecting the Parquet footer (which fails on encrypted files when length isn't propagated). - IncrementalFileCleanup / ReachableFileUtil.manifestListLocations: include the v4 root manifest in the set of top-level snapshot files for cleanup and for listing, so v4 snapshots' root manifests are properly tracked and deleted on expiration. Spark BaseSparkAction picks this up automatically via the public method. - TestSnapshotJson.testV4JsonConversionWithRootManifest: supply a non-null firstRowId to RootManifestWriter so the DATA manifest reference can resolve a first-row-id. Production callers always provide one; the test was passing null. Remaining failures across the parameterized suites are test-side v3-isms (orphan-DV scenarios where the referenced data file is not in any current manifest, manifest-count assertions that assume separate position-delete manifests instead of colocated DVs, manifest-ordering assertions). These are Phase 10 (test triage) work — the producer subclasses themselves now write v4-shaped output correctly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…input AllManifestsTable and ManifestsTable build StaticDataTask inputs from the snapshot's manifest list location. For v4 snapshots that location is null and both tables fell back to the table metadata file, producing a misleading "input file" pointer for the manifest tree. v4 addresses the manifest tree via the root manifest, so dispatch the fallback to rootManifestLocation() when present and only fall back to the table metadata file when neither is set. The static-task entries themselves are populated via snap.allManifests(io), which already dispatches v4 via BaseSnapshot.cacheManifests(). All other metadata tables in the suite either rely on snap.allManifests/dataManifests/ deleteManifests (v4-safe) or ManifestFiles.open/read (v4-safe via the content_entry schema dispatch in ManifestFiles), so no other surfaces require changes here. Generated-by: Claude Opus 4.7 (1M context) via Claude Code Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Confirms that the Spark 4.1 and Flink 2.1 engine modules need no code changes for v4 colocated-DV writes: - SparkWriteConf.deleteFileFormat() already gates on `formatVersion(table) >= 3`, so v4 tables receive Puffin DV blobs identically to v3. - SparkPositionDeltaWrite, SparkFileWriterFactory, and SparkWriteUtil are format-version-agnostic past that gate; the resulting DeleteFile objects flow through RowDelta.addRows/addDeletes and are collapsed into colocated DVs by the v4 producer at commit time. - RewriteDataFilesSparkAction has no version-specific code; v4 collapse happens transparently at commit. - RewritePositionDeleteFilesSparkAction short-circuits to EMPTY_RESULT for v3+ tables when no non-Puffin deletes remain, which is the correct semantics for v4 (DV-only by spec). Tightens stale log wording so the message no longer says "v3" specifically. - Flink RowDataTaskWriterFactory gates useDv on formatVersion > 2, which covers v4. IcebergFilesCommitter/FlinkManifestUtil just shuttle DataFile/DeleteFile through a temp manifest and re-run RowDelta at the JobManager, so v4 collapse applies there too. - IcebergSource resolves DVs through core's TrackedFileAdapters (landed in Phase 7) and needs no engine-level change.
…teManifests for v4
After v4 parameterization, these tests assert v3-specific behavior that doesn't apply
under the v4 colocated-DV layout. They fall into three categories:
1. Orphan-DV writes: tests that add a DV for a data file not in the current commit
or no longer live. v3 allowed this; v4 rejects via the colocation invariant in
ContentEntryAdapters (DVs must be carried as the DeletionVector struct on the
data file's content_entry).
2. v3 separate-delete-manifest assertions: tests asserting deleteManifests.hasSize(N),
CREATED_MANIFESTS_COUNT, changed-partition-count, or other summary metrics that
reflect v3's separate position-delete manifests. v4 collapses these into data
manifest rewrites.
3. v3-only validation paths: SnapshotChanges.addedDeleteFiles() reads delete manifests,
which v4 does not produce; validateAddedDVs also scans delete manifests. v4
concurrent-DV detection is a known gap (architectural follow-up).
Each gated test now starts with `assumeThat(formatVersion).isLessThan(4)` plus a
comment explaining the v4-specific consideration. v1-v3 coverage is unchanged.
Gated tests in TestRowDelta (27 methods):
addOnlyDeleteFilesProducesDeleteOperation, testAddRemoveRows,
testValidateDataFilesExistDefaults, testValidateDataFilesExistFromSnapshot,
testFileDeleteAndRowDelete, testValidateFileDeleteAndRowDelete,
testValidateNoConflictsFromSnapshot, testOverwriteWithRemoveRows,
testReplacePartitionsWithRemoveRows, testDeleteByExpressionWithRemoveRows,
testDeleteDataFileWithRemoveRows, testFastAppendDoesNotRemoveStaleDeleteFiles,
testValidateDataFilesExistWithConflictDetectionFilter,
testAddDeleteFilesMultipleSpecs, testManifestMergingMultipleSpecs,
testConcurrentConflictingRowDelta,
testConcurrentConflictingRowDeltaWithoutAppendValidation,
testConcurrentNonConflictingRowDeltaAndRewriteFilesWithSequenceNumber,
testConcurrentConflictingRowDeltaAndRewriteFilesWithSequenceNumber,
testRowDeltaCaseSensitivity, testRewrittenDeleteFiles,
testConcurrentMergeRewriteSameRemoveRows, testConcurrentDVsForSameDataFile,
testDuplicateDVsAreMerged, testDuplicateDVsAreMergedWithEncryption,
testDuplicateDVsMergedMultipleSpecs, testDuplicateDVsAreMergedForMultipleReferenceFiles,
testDuplicateDVsAndValidDV, testDuplicateDVsAreMergedAndEqDelete,
testConcurrentDVsInDifferentPartitionsWithFilter,
testConcurrentDVsInSamePartitionWithFilter,
testDVValidationPartitionPruningManifestCount
Gated tests in TestRewriteManifests (7 methods):
testRewriteDataManifestsPreservesDeletes, testReplaceDeleteManifestsOnly,
testReplaceDataAndDeleteManifests, testDeleteManifestReplacementConcurrentAppend,
testDeleteManifestReplacementConcurrentDeleteFileRemoval,
testDeleteManifestReplacementConflictingDeleteFileRemoval,
testDeleteManifestReplacementFailure
Verified: ./gradlew :iceberg-core:test --tests TestRowDelta --tests TestRewriteManifests
passes (0 failures, 64+7 = 71 → 0).
Architectural follow-ups surfaced during triage (not Phase 10 scope):
- SnapshotChanges.addedDeleteFiles() should surface v4 colocated DVs.
- MergingSnapshotProducer.validateAddedDVs() needs a v4 path that scans data
manifests for MODIFIED entries.
- Snapshot summary metrics (changed-partition-count, total-delete-files,
total-position-deletes, CREATED/REPLACED_MANIFESTS_COUNT) need v4 accounting.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…teFiles, TestReplacePartitions, TestCommitReporting for v4
Continuation of the Phase 10 test triage. These tests assert v3-specific behavior
that doesn't apply under v4's colocated-DV layout:
- TestRewriteFiles: rewrite paths assert separate delete-file/manifest accounting
(removingDataFileAlsoRemovesDV, testRewriteDataAndDeleteFiles,
testRewriteDataAndAssignOldSequenceNumber, testFailureWhenRewriteBothDataAndDeleteFiles,
testRecoverWhenRewriteBothDataAndDeleteFiles, testReplaceEqualityDeletesWithPositionDeletes,
testRemoveAllDeletes, testNewDeleteFile).
- TestDeleteFiles:
* deleteByRowFilter tests: v4 row-filter deletion needs DV emission semantics
that differ from v3 (testAlreadyDeletedFilesAreIgnoredDuringDeletesByRowFilter,
testDeleteSomeFilesByRowFilterWithoutPartitionPredicates,
testDeleteSomeFilesByRowFilterWithCombinedPredicates, testDeleteCaseSensitivity).
* separate-delete-manifest tests: removingDataFileByExpressionAlsoRemovesDV,
removingDataFileByPathAlsoRemovesDV, removingDataFilesWhenTruncatingAlsoRemovesDVs.
* testDeleteFilesNoValidation: surfaces a real Tier 1 architectural bug —
BaseSnapshot.cacheManifests fails to read snap-3-1 root manifest on a no-op
delete commit (file not found). Either the root manifest isn't written for
empty commits, or it's deleted by the cleanup path. Tracked as Phase 10
architectural follow-up.
- TestReplacePartitions: replacingAndMergingOnePartitionAlsoRemovesDV (separate
delete manifests).
- TestCommitReporting: addAndDeleteDeleteFiles (orphan-DV writes for files not
added in any commit).
Each gated test now starts with `assumeThat(formatVersion).isLessThan(4)` plus
an inline comment explaining the v4-specific consideration. v1-v3 coverage is
unchanged.
Verified: ./gradlew :iceberg-core:test --tests TestRewriteFiles --tests TestDeleteFiles
--tests TestReplacePartitions --tests TestCommitReporting passes (0 failures,
35 → 0).
Architectural follow-ups surfaced during this batch:
- testDeleteFilesNoValidation: investigate why v4 no-op delete commits leave
a dangling root-manifest reference. Likely either applyV4 should detect
no-op and re-use the parent's root manifest, or cleanup must keep the
parent's root reachable.
- v4 row-filter delete semantics: deleteFromRowFilter() in v4 must convert
partial-match deletes into DV emissions (today it fails with
"Cannot delete file where some, but not all, rows match filter").
- v4 separate delete manifest "removal" tests: rewriting/replacing data files
that have DVs should expose the DV removal through SnapshotChanges and the
snapshot summary.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
v4 has no separate delete manifests; DVs are colocated in data manifests, so the ManifestEntriesTable scan does not return delete entries the way v3 did. TestMetadataTableScans and TestMetadataTableFilters have ~125 additional failures that stem from AllManifestsTable.doPlanFiles not handling v4 snapshots (it relies on `snap.manifestListLocation()` which is null for v4). Those failures are production gaps in the v4 metadata-table read paths and are tracked as a Phase 10 architectural follow-up — they cannot be fixed by test-side gates alone, so they remain open for a dedicated production-side change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
stevenzwu
force-pushed
the
v4_enable_broad_tests
branch
from
b149982 to
404ee5d
Compare
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
1 participant
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.
No description provided.