Core: Fix incorrect delete manifest pruning in entries metadata tables - #17440
Core: Fix incorrect delete manifest pruning in entries metadata tables#17440yangshangqing95 wants to merge 1 commit into
Conversation
| private boolean containsOnlyFileContent(int fileContentId) { | ||
| return ManifestContent.DATA == manifestContent && FileContent.DATA.id() == fileContentId; | ||
| } |
There was a problem hiding this comment.
can add a comment to highlight why notEq only evaluate the data manifest: a delete manifest may hold both position and equality deletes, so it never has a single value that notEq could rule out.
| return ManifestContent.DATA == manifestContent && FileContent.DATA.id() == fileContentId; | ||
| } | ||
|
|
||
| private <T> boolean containsOnlyFileContents(Set<T> fileContentIds) { |
There was a problem hiding this comment.
nit: containsAllPossibleFileContents
| if (fileContent(ref)) { | ||
| Literal<Integer> intLit = lit.to(Types.IntegerType.get()); | ||
| if (!contentMatch(intLit.value())) { | ||
| if (!mayContainFileContent(intLit.value())) { |
There was a problem hiding this comment.
for in and eq predicate, can we also cover the case where literal is outside the valid value range to ensure coverage? like Expressions.in("data_file.content", 3, 4) shall not scan for any manifests
| } | ||
|
|
||
| private void assertManifestPlanned(Table metadataTable, Expression filter, String manifestPath) { | ||
|
|
There was a problem hiding this comment.
nit, extra newline in LINE 207 and 218
szehon-ho
left a comment
szehon-ho
left a comment
There was a problem hiding this comment.
Traced the pruning logic end-to-end and it's sound: for a DATA manifest content is always 0, so != 0 and NOT IN {…0…} are correctly unsatisfiable; for a DELETES manifest content ranges over {1, 2}, so != x is never unsatisfiable and NOT IN S is unsatisfiable only when S ⊇ {1, 2}. I also confirmed the premise: MergingSnapshotProducer groups new delete files only by spec id, so position and equality deletes really do land in the same manifest.
mayContainFileContent is behaviorally identical to the old contentMatch for every reachable input, so eq/in are a pure rename and the entire behavior change is confined to notEq/notIn. I hand-checked all four existing testEntriesTableDataFileContent* tests in TestMetadataTableScans, including the out-of-range literal cases, and none of them change result.
My comments are all on the test side — the notIn fix currently ships without coverage, and neither test builds the mixed delete manifest that motivates the change.
| String deleteManifestPath = | ||
| Iterables.getOnlyElement(table.currentSnapshot().deleteManifests(table.io())).path(); | ||
|
|
||
| Expression filter = Expressions.notIn("data_file.content", FileContent.EQUALITY_DELETES.id()); |
There was a problem hiding this comment.
This test doesn't exercise notIn. A single-literal NOT_IN is collapsed into a NOT_EQ bound predicate by UnboundPredicate.bindInOperation, so this routes through notEq/containsOnlyFileContent and is an exact duplicate of testNotEqualRetainsPositionDeleteManifest.
That leaves the new containsOnlyFileContents(Set) with no coverage for the behavior it changes — the two multi-literal cases in TestMetadataTableScans (notIn(1, 2) and notIn(0, 1, 2)) give identical results under the old and new code, so they pass either way. A discriminating case needs a set containing exactly one of the two delete content ids, e.g. notIn("data_file.content", 2, 3) against a delete manifest: the old anyMatch prunes it, the new code retains it.
| } | ||
|
|
||
| @TestTemplate | ||
| public void testNotEqualRetainsPositionDeleteManifest() { |
There was a problem hiding this comment.
Both retention tests create a manifest holding only a position delete (or a DV on v3) and assert at the manifest-planning level, so the mixed position+equality manifest that motivates the fix is never actually built, and the user-visible symptom is never shown.
TestMetadataTableScans.preparePartitionedTable() already produces exactly that manifest — one newRowDelta adding fileADeletes()/fileBDeletes() alongside FILE_C2_DELETES/FILE_D2_DELETES. Against that setup, notEqual("data_file.content", 1) silently drops two real equality-delete rows from the entries output before this fix and returns them after. That's the strongest regression test available here and it costs one method.
| assertManifestNotPlanned(new ManifestEntriesTable(table), filter, dataManifestPath); | ||
| } | ||
|
|
||
| private void assertManifestPlanned(Table metadataTable, Expression filter, String manifestPath) { |
There was a problem hiding this comment.
These two helpers reimplement scannedPaths(TableScan), which TestMetadataTableScans already inherits from MetadataTableScanTestBase (they also do a redundant double copy: ImmutableList.copyOf(...).stream().collect(toImmutableList())).
Since TestMetadataTableScans already owns testEntriesTableDataFileContentEq/NotEq/In/NotIn, extending those with the equality-delete cases would reuse the helper, get the mixed delete manifest for free, and keep all data_file.content coverage in one place.
|
|
||
| Expression filter = Expressions.notEqual("data_file.content", FileContent.DATA.id()); | ||
|
|
||
| assertManifestNotPlanned(new ManifestEntriesTable(table), filter, dataManifestPath); |
There was a problem hiding this comment.
Minor inconsistency: this one checks only ManifestEntriesTable, while the other two tests assert against both ManifestEntriesTable and AllEntriesTable.
| || FileContent.EQUALITY_DELETES.id() == fileContentId; | ||
| } | ||
|
|
||
| // Be conservative for an unknown manifest content. |
There was a problem hiding this comment.
This branch is unreachable — ManifestContent is a closed enum of DATA and DELETES, and ManifestFile.content() never returns null. A switch on the enum with the safe value in default would keep the same conservatism while letting the compiler flag a newly added constant; as written, all three helpers need a hand-audited fallback. Low stakes either way, since true for may-contain and false for contains-only are both the safe directions.
3 participants
This PR fixes incorrect pruning for negative data_file.content predicates in the entries and all_entries metadata tables.
A delete manifest may contain either position delete files or equality delete files. The existing evaluator treats both as ManifestContent.DELETES, which can incorrectly prune delete manifests for predicates such as:
The fix uses:
may-contain semantics for = and IN
contains-only semantics for != and NOT IN
This prevents false-negative results while preserving safe manifest pruning.
Fixes #17439