feat(manifest): add ManifestFilterManager and ManifestMergeManager#652
feat(manifest): add ManifestFilterManager and ManifestMergeManager#652gty404 wants to merge 1 commit into
Conversation
Implement two manifest management classes for table write operations: - ManifestFilterManager: filters manifest entries by row filter expression, file path, or partition value; supports FailMissingDeletePaths validation. Rewrites manifests that contain matching files, marking entries as DELETED; passes through manifests that cannot contain matching files unchanged. - ManifestMergeManager: merges small manifests using greedy bin-packing, grouping by partition_spec_id (manifests with different specs are never merged). Oversized manifests pass through unchanged. ADDED entries from prior manifests become EXISTING when merged (matching Java semantics).
| for (const auto& de : delete_exprs_) { | ||
| ICEBERG_ASSIGN_OR_RAISE(auto* eval, GetMetricsEvaluator(metadata, spec_id, de)); | ||
| ICEBERG_ASSIGN_OR_RAISE(auto matches, eval->Evaluate(file)); | ||
| if (matches) return true; |
There was a problem hiding this comment.
This row-filter delete path is broader than Java's behavior. Java's ManifestFilterManager first computes a partition residual and then uses both inclusive and strict metrics: files are deleted only when rowsMustMatch is true, and data files where only some rows may match raise Cannot delete file where some, but not all, rows match filter. Here, an inclusive might match result is enough to mark the whole file as DELETED, so a file whose bounds straddle the predicate would be incorrectly removed instead of rejected.
| const TableMetadata& metadata, const std::shared_ptr<Snapshot>& base_snapshot, | ||
| const ManifestWriterFactory& writer_factory) { | ||
| // No base snapshot → nothing to filter | ||
| if (!base_snapshot) return std::vector<ManifestFile>{}; |
There was a problem hiding this comment.
Java still validates required deletes when the manifest list is null or empty: filterManifests calls validateRequiredDeletes() before returning. With FailMissingDeletePaths() set, this early return silently succeeds for a missing path on an empty/no-base snapshot, while Java reports Missing required files to delete.
| bool ManifestFilterManager::CanContainDeletedFiles(const ManifestFile& manifest, | ||
| const TableMetadata& metadata) { | ||
| // A manifest with no live files cannot contain files to delete. | ||
| bool has_live = (manifest.added_files_count.value_or(0) > 0) || |
There was a problem hiding this comment.
This treats missing manifest counts as zero live files. In Iceberg/Java, null addedFilesCount or existingFilesCount means the count is unknown and ManifestFile.hasAddedFiles/hasExistingFiles returns true. Skipping such manifests can miss deletes for older or compatible metadata where counts are not populated.
| all.insert(all.end(), new_manifests.begin(), new_manifests.end()); | ||
| all.insert(all.end(), existing_manifests.begin(), existing_manifests.end()); | ||
|
|
||
| if (!merge_enabled_ || std::cmp_less(all.size(), min_count_to_merge_)) { |
There was a problem hiding this comment.
This global min_count_to_merge_ check does not match Java. Java only applies minCountToMerge to the bin containing the first/newest manifest; older bins that do not contain first can still be merged even when the total input count is below the threshold. Returning all manifests here can skip merges Java would perform.
| std::vector<ManifestFile> result; | ||
|
|
||
| for (const auto& manifest : group) { | ||
| if (manifest.manifest_length >= target_size_bytes_) { |
There was a problem hiding this comment.
Java uses ListPacker.packEnd(group, ManifestFile::length) with lookback 1 specifically to preserve manifest order and leave the under-filled bin at the beginning. This forward hand-rolled packing keeps the current small bin open across an oversized manifest, so a sequence like [small, oversized, small] can merge the two small manifests across the oversized one and reorder output relative to Java.
| const ManifestFile& first = all[0]; | ||
|
|
||
| // Group manifests by partition_spec_id — never merge across specs | ||
| std::map<int32_t, std::vector<ManifestFile>> by_spec; |
There was a problem hiding this comment.
Java groups manifests by partition spec using a reverse-order TreeMap. This std::map iterates specs in ascending order, so the merged manifest list order can differ from Java. For v3 tables this ordering is observable because manifest-list writing assigns first-row IDs in output order.
| } | ||
|
|
||
| // The first (newest) manifest governs the per-bin minCountToMerge check. | ||
| const ManifestFile& first = all[0]; |
There was a problem hiding this comment.
Please keep the Java empty-input behavior here before indexing into all. Java ManifestMergeManager.mergeManifests returns the input immediately when the iterator is empty, and commit.manifest.min-count-to-merge=0 is a valid/aggressively-used setting in Java tests. With this implementation, an empty data/delete manifest input plus min_count_to_merge_ <= 0 gets past the size check and reads all[0], which is UB/crash instead of returning an empty manifest list.
|
|
||
| // Path-based check | ||
| if (delete_paths_.count(file.file_path)) { | ||
| pending_paths_.erase(file.file_path); |
There was a problem hiding this comment.
This consumes the required-delete tracking while filtering, which makes FailMissingDeletePaths non-idempotent across retries. Java keeps deletePaths immutable and validateRequiredDeletes recomputes the deleted file set from the filtered manifests on every call, so a retry against a changed base can still fail if the required path is no longer present. Here, once the first filtering pass erases the path from pending_paths_, a later FilterManifests call on the same manager can miss the validation failure because the path stays consumed.
| /// entries are rewritten with those entries marked DELETED. | ||
| /// | ||
| /// The manager is content-agnostic: pass ManifestContent::kData to process data | ||
| /// manifests, or ManifestContent::kDeletes to process delete manifests. |
There was a problem hiding this comment.
If this manager is intended to cover delete manifests, it is still missing the Java DeleteFileFilterManager cleanup semantics. Java sets dropDeleteFilesOlderThan(minDataSequenceNumber) and removeDanglingDeletesFor(filesToBeDeleted), then marks old delete files and dangling DVs as deleted while rewriting delete manifests. This implementation only applies path/partition/row-filter deletes, so a data-file delete or truncate can leave obsolete delete files/DVs live in the delete manifests, diverging from Java behavior and its DV cleanup tests. If delete-manifest cleanup is planned in a later layer, it would be good to avoid advertising kDeletes parity here or make the missing behavior explicit.
| const ManifestFile& first = all[0]; | ||
|
|
||
| // Group manifests by partition_spec_id — never merge across specs | ||
| std::map<int32_t, std::vector<ManifestFile>> by_spec; |
There was a problem hiding this comment.
Could we keep data and delete manifests type-separated in this API? Java has distinct DataFileMergeManager and DeleteFileMergeManager, and MergingSnapshotProducer passes dataManifests/deleteManifests separately. Here the grouping key is only partition_spec_id, while FlushBin creates the writer from the first manifest content; if a caller accidentally gives a data and delete manifest with the same spec in one call, the bin can mix contents and fail mid-rewrite, a state Java typed managers make impossible. Either include content in the grouping/constructor or make the manager content-specific, and add a mixed-content / kDeletes merge test.
| /// \param file_io File IO used to open existing manifests for reading | ||
| /// \param writer_factory Factory to create new ManifestWriter instances | ||
| /// \return The merged manifest list, or an error | ||
| Result<std::vector<ManifestFile>> MergeManifests( |
There was a problem hiding this comment.
Could this manager expose the lifecycle state Java relies on for retries? Java ManifestMergeManager caches merged manifests, reports replacedManifestsCount(), and has cleanUncommitted(committed) so MergingSnapshotProducer can delete manifests produced by failed retry attempts and build created/kept/replaced summary accurately. This API only returns the final vector, so the later snapshot update layer has no way to distinguish committed vs uncommitted merged outputs or count replaced manifests consistently with Java tests such as transaction retry merge cleanup. The same lifecycle surface is also needed on the filter side for rewritten manifests.
| /// Any manifest entry whose file_path matches this path will be marked DELETED. | ||
| /// | ||
| /// \param path The exact file path to delete | ||
| void DeleteFile(std::string_view path); |
There was a problem hiding this comment.
Could we keep the Java file-object delete path in the manager API instead of only accepting paths? Java ManifestFilterManager supports delete(F file) in addition to delete(CharSequence path); RewriteFiles.deleteFile(DataFile/DeleteFile) and StreamingDelete.deleteFile(DataFile) use that path, and the manager then retains file identity/manifest references for filesToBeDeleted(), buildSummary(), duplicate-delete accounting, and delete-manifest DV cleanup. With only DeleteFile(string_view), later layers have to degrade object deletes to paths and lose the state Java relies on. It would be good to add DataFile/DeleteFile object-delete coverage alongside the current path-only tests.
2 participants
Implement two manifest management classes for table write operations:
ManifestFilterManager: filters manifest entries by row filter expression, file path, or partition value; supports FailMissingDeletePaths validation. Rewrites manifests that contain matching files, marking entries as DELETED; passes through manifests that cannot contain matching files unchanged.
ManifestMergeManager: merges small manifests using greedy bin-packing, grouping by partition_spec_id (manifests with different specs are never merged). Oversized manifests pass through unchanged. ADDED entries from prior manifests become EXISTING when merged (matching Java semantics).