-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core: Optimize MergingSnapshotProducer to use referenced manifests to determine if manifest needs to be rewritten #11131
Core: Optimize MergingSnapshotProducer to use referenced manifests to determine if manifest needs to be rewritten #11131
Conversation
Publishing a draft so I can test against entire CI. I need to think more about a good way to benchmark this and if there's even more reasonable optimizations that I can do here Well another possible optimization to think through: If we have both the manifestLocation and the ordinal position of the content file in the manifest AND there are no delete expressions or deletions by pure paths, we can possibly just write the new manifest with entries that are not part of the referenced positions without having to evaluate file paths or predicates against manifest entries. We could just evaluate against the positions (every entry would be compared against the "deleted" pos set) as opposed to file paths/partition values, which should be a bit more performant. |
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java
Outdated
Show resolved
Hide resolved
ad1dd92
to
b230f1e
Compare
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java
Outdated
Show resolved
Hide resolved
47cda8a
to
4099608
Compare
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java
Outdated
Show resolved
Hide resolved
4099608
to
bfed848
Compare
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On testing, even though SparkRewriteDataFilesAction exercises this new path I think it's worth adding a separate test in TestRewriteFiles, or if https://github.com/apache/iceberg/pull/11166/files gets in first can also add a test to RowDelta. The test should explicitly read entries from manifests to delete
bfed848
to
d0f28a6
Compare
be432d6
to
5eece22
Compare
core/src/jmh/java/org/apache/iceberg/ReplaceDeleteFilesBenchmark.java
Outdated
Show resolved
Hide resolved
Seems like after my latest updates to not add to the delete paths if a manifest location is defined, some cherry pick test cases are failing. Taking a look. |
b37854d
to
07a3b1b
Compare
2e583a2
to
e8a87bc
Compare
DeleteFile deleteFile = TestHelpers.deleteFiles(table).iterator().next(); | ||
Path deleteFilePath = new Path(String.valueOf(deleteFile.path())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests changed for the same reason mentioned #11131 (comment)
eb3e848
to
6e71f7f
Compare
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java
Outdated
Show resolved
Hide resolved
// assuming that the manifest does not have any live entries or aged out deletes | ||
Set<String> manifestLocations = | ||
manifests.stream().map(ManifestFile::path).collect(Collectors.toSet()); | ||
boolean trustReferencedManifests = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spotless formats this in a weird way. I wonder if we can play around with the names to stay on 1 line and/or potentially add a helper method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried a bit here on naming, but unfortunately couldn't get a shorter name which accurately captures what this variable represents. I just went ahead with a helper method, since I think really someone reading the code in most cases generally just needs to know what it means to trust the referenced manifests rather than the "how". If they need to know the conditions for trust, it seems reasonable to just read the helper method logic.
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java
Outdated
Show resolved
Hide resolved
…o determine if a given manifest needs to be rewritten or not
eab6906
to
fb5a573
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Awesome work, @amogh-jahagirdar!
} | ||
|
||
rowDelta.commit(); | ||
|
||
this.deleteFiles = generatedDeleteFiles; | ||
List<DeleteFile> deleteFilesReadFromManifests = Lists.newArrayList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a bit hard to follow the way we generate these values (like why use a map if only checking the key?). What if we restructure this logic a bit given that we have to read the manifests anyway?
RowDelta rowDelta = table.newRowDelta();
for (int ordinal = 0; ordinal < numFiles; ordinal++) {
...
}
rowDelta.commit();
int replacedDeleteFilesCount = (int) Math.ceil(numFiles * (percentDeleteFilesReplaced / 100.0));
List<DeleteFile> oldDeleteFiles = Lists.newArrayListWithExpectedSize(replacedDeleteFilesCount);
List<DeleteFile> newDeleteFiles = Lists.newArrayListWithExpectedSize(replacedDeleteFilesCount);
try (CloseableIterable<FileScanTask> tasks = table.newScan().planFiles()) {
for (FileScanTask task : Iterables.limit(tasks, replacedDeleteFilesCount)) {
DeleteFile oldDeletes = Iterables.getOnlyElement(task.deletes());
oldDeleteFiles.add(oldDeletes);
DeleteFile newDeletes = FileGenerationUtil.generatePositionDeleteFile(table, task.file());
newDeleteFiles.add(newDeletes);
}
}
this.deleteFilesToReplace = oldDeleteFiles;
this.pendingDeleteFiles = newDeleteFiles;
@@ -69,6 +70,7 @@ public String partition() { | |||
private final Map<Integer, PartitionSpec> specsById; | |||
private final PartitionSet deleteFilePartitions; | |||
private final Set<F> deleteFiles = newFileSet(); | |||
private final Set<String> manifestsReferencedForDeletes = Sets.newHashSet(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any shorter names like manifestsWithDeletes
or deleteFileManifests
?
A few calls below split into multiple lines.
@@ -185,6 +198,14 @@ List<ManifestFile> filterManifests(Schema tableSchema, List<ManifestFile> manife | |||
return ImmutableList.of(); | |||
} | |||
|
|||
// Use the current set of referenced manifests as a source of truth when it's a subset of all | |||
// manifests and all removals which were performed reference manifests. | |||
// If a manifest is not in the trusted referenced set and has no live files, this means that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first sentence is very clear. I am not sure I follow the bit about "and has no live files" in the second sentence, as the check for live files is done further. Even if we want to mention live files, shall and
become or
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me reword a bit more, I was trying to express the additional condition that even if a file is not in the referenced manifests, if there are live files that still means we need to rewrite the manifest.
// Use the current set of referenced manifests as a source of truth when it's a subset of all | ||
// manifests and all removals which were performed reference manifests. | ||
// If a manifest is not in the trusted referenced set and has no live files, this means that the | ||
// manifest has no deleted entries and does not need to be rewritten |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing .
for consistency with the first sentence?
// manifests and all removals which were performed reference manifests. | ||
// If a manifest is not in the trusted referenced set and has no live files, this means that the | ||
// manifest has no deleted entries and does not need to be rewritten | ||
Set<String> manifestLocations = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Why not have this inside canTrustManifestReferences
?
@@ -327,62 +354,71 @@ private ManifestFile filterManifest(Schema tableSchema, ManifestFile manifest) { | |||
// this assumes that the manifest doesn't have files to remove and streams through the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the empty line above this comment still has a purpose, given it is one block now.
} | ||
|
||
@SuppressWarnings({"CollectionUndefinedEquality", "checkstyle:CyclomaticComplexity"}) | ||
private boolean manifestHasDeletedFiles( | ||
PartitionAndMetricsEvaluator evaluator, ManifestReader<F> reader) { | ||
if (manifestsReferencedForDeletes.contains(reader.file().location())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was my bad to suggest reader.file().location()
. It will be fragile as the location may undergo some validation or parsing in FileIO
, which we don't control here. Probably better to pass ManifestFile
to this method and simply use manifest.path()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah not at all, my bad for missing this. Yeah we shouldn't touch reader.file() and just use the manifest.path
…o determine if a given manifest needs to be rewritten or not
fb5a573
to
d5147a4
Compare
Thanks @aokolnychyi @rdblue for reviewing, will go ahead and merge! |
… determine if manifest needs to be rewritten (apache#11131)
I have a compaction test ( The tests creates an Iceberg table with 2 snapshots with delete files each:
Then the test creates a compaction commit with the Before this change (#11131) the resulting snapshot contained a single data file, and a single delete file. The table content is: DF3, EQ2 Is this change intentional? Data wise the result is correct in both cases, as no data files are remaining for which the delete files need to be applied, but the new result is definitely suboptimal. |
@pvary I'll double check that test but my suspicion is you're seeing the behavior from this particular change #11131 (comment), this has the rationale. My suspicion is that the test used to rely on the eager rewriting of manifests with only aged out deletes, and that's no longer the case. If that's the case, what's happening is that we're not eagerly rewriting manifests which only have aged out deletes since it's not required for correctness and it's best to avoid any additional work and risk failure for things not related to metadata correctness (of course this does tradeoff the extra metadata in storage for a period, until that manifest gets impacted for another write). Important note is that, if a manifest needs to be rewritten for other purposes and has aged out deletes, the aged out deletes will be removed from metadata. Let me take a deeper look though to confirm that's what happening for the test case |
… determine if manifest needs to be rewritten (apache#11131)
This change optimizes merging snapshot producer logic to determine if a file has been deleted in a manifest in the case
deleteFile(ContentFile file)
is called; optimizing this logic means that determining if a manifest needs to be rewritten is now optimized.Previously determining a manifest file needs to be rewritten has 2 high level steps:
The optimization in this PR will optimize step 1 in the case
deleteFile(ContentFile file)
is called by keeping track of the data/delete file's manifests and the position in the manifest that is being deleted. Using this the logic for doing the first pass over the manifest is no longer necessary since the presence of a referenced manifest means that manifest must be rewritten.Before