dramaticlly commented on code in PR #17440:
URL: https://github.com/apache/iceberg/pull/17440#discussion_r3686414279
##########
core/src/main/java/org/apache/iceberg/BaseEntriesTable.java:
##########
@@ -255,15 +254,31 @@ private <T> boolean fileContent(BoundReference<T> ref) {
return ref.fieldId() == DataFile.CONTENT.fieldId();
}
- private boolean contentMatch(Integer fileContentId) {
- if (FileContent.DATA.id() == fileContentId) {
- return ManifestContent.DATA.id() == manifestContentId;
- } else if (FileContent.EQUALITY_DELETES.id() == fileContentId
- || FileContent.POSITION_DELETES.id() == fileContentId) {
- return ManifestContent.DELETES.id() == manifestContentId;
- } else {
- return false;
+ private boolean mayContainFileContent(int fileContentId) {
+ if (ManifestContent.DATA == manifestContent) {
+ return FileContent.DATA.id() == fileContentId;
+ } else if (ManifestContent.DELETES == manifestContent) {
+ return FileContent.POSITION_DELETES.id() == fileContentId
+ || FileContent.EQUALITY_DELETES.id() == fileContentId;
+ }
+
+ // Be conservative for an unknown manifest content.
+ return true;
+ }
+
+ private boolean containsOnlyFileContent(int fileContentId) {
+ return ManifestContent.DATA == manifestContent &&
FileContent.DATA.id() == fileContentId;
+ }
Review 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.
##########
core/src/test/java/org/apache/iceberg/TestEntriesMetadataTable.java:
##########
@@ -152,4 +154,73 @@ public void testEntriesTableWithDeleteManifests() {
.as("Should contain 1 delete file record")
.isEqualTo(1);
}
+
+ @TestTemplate
+ public void testNotEqualRetainsPositionDeleteManifest() {
+ assumeThat(formatVersion).as("Only V2+ tables support
deletes").isGreaterThanOrEqualTo(2);
+
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newRowDelta().addDeletes(fileADeletes()).commit();
+
+ String deleteManifestPath =
+
Iterables.getOnlyElement(table.currentSnapshot().deleteManifests(table.io())).path();
+
+ Expression filter =
+ Expressions.notEqual("data_file.content",
FileContent.EQUALITY_DELETES.id());
+
+ assertManifestPlanned(new ManifestEntriesTable(table), filter,
deleteManifestPath);
+ assertManifestPlanned(new AllEntriesTable(table), filter,
deleteManifestPath);
+ }
+
+ @TestTemplate
+ public void testNotInRetainsPositionDeleteManifest() {
+ assumeThat(formatVersion).as("Only V2+ tables support
deletes").isGreaterThanOrEqualTo(2);
+
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newRowDelta().addDeletes(fileADeletes()).commit();
+
+ String deleteManifestPath =
+
Iterables.getOnlyElement(table.currentSnapshot().deleteManifests(table.io())).path();
+
+ Expression filter = Expressions.notIn("data_file.content",
FileContent.EQUALITY_DELETES.id());
+
+ assertManifestPlanned(new ManifestEntriesTable(table), filter,
deleteManifestPath);
+ assertManifestPlanned(new AllEntriesTable(table), filter,
deleteManifestPath);
+ }
+
+ @TestTemplate
+ public void testNotEqualPrunesDataManifest() {
+ assumeThat(formatVersion).as("Only V2+ tables support
deletes").isGreaterThanOrEqualTo(2);
+
+ table.newAppend().appendFile(FILE_A).commit();
+ table.newRowDelta().addDeletes(fileADeletes()).commit();
+
+ String dataManifestPath =
+
Iterables.getOnlyElement(table.currentSnapshot().dataManifests(table.io())).path();
+
+ Expression filter = Expressions.notEqual("data_file.content",
FileContent.DATA.id());
+
+ assertManifestNotPlanned(new ManifestEntriesTable(table), filter,
dataManifestPath);
+ }
+
+ private void assertManifestPlanned(Table metadataTable, Expression filter,
String manifestPath) {
+
Review Comment:
nit, extra newline in LINE 207 and 218
##########
core/src/main/java/org/apache/iceberg/BaseEntriesTable.java:
##########
@@ -203,7 +203,7 @@ public <T> Boolean gtEq(BoundReference<T> ref, Literal<T>
lit) {
public <T> Boolean eq(BoundReference<T> ref, Literal<T> lit) {
if (fileContent(ref)) {
Literal<Integer> intLit = lit.to(Types.IntegerType.get());
- if (!contentMatch(intLit.value())) {
+ if (!mayContainFileContent(intLit.value())) {
Review 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
##########
core/src/main/java/org/apache/iceberg/BaseEntriesTable.java:
##########
@@ -255,15 +254,31 @@ private <T> boolean fileContent(BoundReference<T> ref) {
return ref.fieldId() == DataFile.CONTENT.fieldId();
}
- private boolean contentMatch(Integer fileContentId) {
- if (FileContent.DATA.id() == fileContentId) {
- return ManifestContent.DATA.id() == manifestContentId;
- } else if (FileContent.EQUALITY_DELETES.id() == fileContentId
- || FileContent.POSITION_DELETES.id() == fileContentId) {
- return ManifestContent.DELETES.id() == manifestContentId;
- } else {
- return false;
+ private boolean mayContainFileContent(int fileContentId) {
+ if (ManifestContent.DATA == manifestContent) {
+ return FileContent.DATA.id() == fileContentId;
+ } else if (ManifestContent.DELETES == manifestContent) {
+ return FileContent.POSITION_DELETES.id() == fileContentId
+ || FileContent.EQUALITY_DELETES.id() == fileContentId;
+ }
+
+ // Be conservative for an unknown manifest content.
+ return true;
+ }
+
+ private boolean containsOnlyFileContent(int fileContentId) {
+ return ManifestContent.DATA == manifestContent &&
FileContent.DATA.id() == fileContentId;
+ }
+
+ private <T> boolean containsOnlyFileContents(Set<T> fileContentIds) {
Review Comment:
nit: containsAllPossibleFileContents
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
