Flink: add append capability to dynamic iceberg sink (#14526) #14559
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ | |
| import org.apache.flink.annotation.Internal; | ||
| import org.apache.flink.api.connector.sink2.Committer; | ||
| import org.apache.flink.core.io.SimpleVersionedSerialization; | ||
| import org.apache.iceberg.AppendFiles; | ||
| import org.apache.iceberg.ManifestFile; | ||
| import org.apache.iceberg.ReplacePartitions; | ||
| import org.apache.iceberg.RowDelta; | ||
|
|
@@ -302,30 +303,58 @@ private void commitDeltaTxn( | |
| CommitSummary summary, | ||
| String newFlinkJobId, | ||
| String operatorId) { | ||
| if (summary.deleteFilesCount() == 0) { | ||
| // To be compatible with iceberg format V1. | ||
| AppendFiles appendFiles = table.newAppend().scanManifestsWith(workerPool); | ||
| for (List<WriteResult> resultList : pendingResults.values()) { | ||
| for (WriteResult result : resultList) { | ||
| Preconditions.checkState( | ||
| result.referencedDataFiles().length == 0, | ||
| "Should have no referenced data files for append."); | ||
| Arrays.stream(result.dataFiles()).forEach(appendFiles::appendFile); | ||
| } | ||
| } | ||
| String description = "append"; | ||
|
||
|
|
||
| // fail all commits as really its only one | ||
| commitOperation( | ||
| table, | ||
| branch, | ||
| appendFiles, | ||
| summary, | ||
| description, | ||
|
||
| newFlinkJobId, | ||
| operatorId, | ||
| pendingResults.lastKey()); | ||
| } else { | ||
| for (Map.Entry<Long, List<WriteResult>> e : pendingResults.entrySet()) { | ||
| long checkpointId = e.getKey(); | ||
| List<WriteResult> writeResults = e.getValue(); | ||
|
|
||
| RowDelta rowDelta = table.newRowDelta().scanManifestsWith(workerPool); | ||
| for (WriteResult result : writeResults) { | ||
| // Row delta validations are not needed for streaming changes that write equality deletes. | ||
| // Equality deletes are applied to data in all previous sequence numbers, so retries may | ||
| // push deletes further in the future, but do not affect correctness. Position deletes | ||
| // committed to the table in this path are used only to delete rows from data files that | ||
| // are | ||
| // being added in this commit. There is no way for data files added along with the delete | ||
| // files to be concurrently removed, so there is no need to validate the files referenced | ||
| // by | ||
| // the position delete files that are being committed. | ||
| Arrays.stream(result.dataFiles()).forEach(rowDelta::addRows); | ||
| Arrays.stream(result.deleteFiles()).forEach(rowDelta::addDeletes); | ||
| } | ||
|
|
||
| // Every Flink checkpoint contains a set of independent changes which can be committed | ||
| // together. While it is technically feasible to combine append-only data across | ||
| // checkpoints, | ||
| // for the sake of simplicity, we do not implement this (premature) optimization. Multiple | ||
| // pending checkpoints here are very rare to occur, i.e. only with very short checkpoint | ||
| // intervals or when concurrent checkpointing is enabled. | ||
| commitOperation( | ||
| table, branch, rowDelta, summary, "rowDelta", newFlinkJobId, operatorId, checkpointId); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ | |
| import org.apache.flink.annotation.Internal; | ||
| import org.apache.flink.api.connector.sink2.Committer; | ||
| import org.apache.flink.core.io.SimpleVersionedSerialization; | ||
| import org.apache.iceberg.AppendFiles; | ||
| import org.apache.iceberg.ManifestFile; | ||
| import org.apache.iceberg.ReplacePartitions; | ||
| import org.apache.iceberg.RowDelta; | ||
|
|
@@ -302,30 +303,58 @@ private void commitDeltaTxn( | |
| CommitSummary summary, | ||
| String newFlinkJobId, | ||
| String operatorId) { | ||
|
Contributor
There was a problem hiding this comment. Can we keep the loop structure? We will need it for both types of snapshots. This should work: for (Map.Entry<Long, List<WriteResult>> e : pendingResults.entrySet()) {
long checkpointId = e.getKey();
List<WriteResult> writeResults = e.getValue();
boolean appendOnly = true;
for (WriteResult writeResult : writeResults) {
if (writeResult.deleteFiles().length > 0) {
appendOnly = false;
break;
}
}
final SnapshotUpdate snapshotUpdate;
if (appendOnly) {
AppendFiles appendFiles = table.newAppend().scanManifestsWith(workerPool);
for (WriteResult result : writeResults) {
Arrays.stream(result.dataFiles()).forEach(appendFiles::appendFile);
}
snapshotUpdate = appendFiles
} else {
RowDelta rowDelta = table.newRowDelta().scanManifestsWith(workerPool);
for (WriteResult result : writeResults) {
Arrays.stream(result.dataFiles()).forEach(rowDelta::addRows);
Arrays.stream(result.deleteFiles()).forEach(rowDelta::addDeletes);
}
snapshotUpdate = rowDelta;
}
commitOperation(
table,
branch,
snapshotUpdate,
summary,
appendOnly ? "append" : "rowDelta",
newFlinkJobId,
operatorId,
checkpointId);
}
Author
There was a problem hiding this comment. this looks good - only thing left is the checkState for result.referencedDataFiles().length == 0 (which exists in IcebergSink) which I will add to the loop checking it is appendOnly |
||
| if (summary.deleteFilesCount() == 0) { | ||
|
Contributor
There was a problem hiding this comment. I'm not sure about the granularity of this value, as every pending result could contain or not contain deletes. Probably best to check the WriteResults directly.
Contributor
There was a problem hiding this comment. The This is how it is calculated: And internally:
Contributor
There was a problem hiding this comment. The value is correct but we commit at checkpoint level and the delete file count is done across checkpoints. Strictly speaking, there could be both append-only checkpoints and overwrite checkpoints as part of
Contributor
There was a problem hiding this comment. Interesting decision.
Contributor
There was a problem hiding this comment. When we fixed #14182, we decided not to do that. In fact, you proposed not to do that: #14182 (comment) 🤓 IMHO this kind of optimization is a bit premature. In practice it is rare to even have multiple pending checkpoints.
Contributor
There was a problem hiding this comment. Also, as I have mentioned in the comment, this could cause issues if "replacePartition" is used
Contributor
There was a problem hiding this comment. So we should commit them 1-by-1 |
||
| // To be compatible with iceberg format V1. | ||
| AppendFiles appendFiles = table.newAppend().scanManifestsWith(workerPool); | ||
| for (List<WriteResult> resultList : pendingResults.values()) { | ||
|
Contributor
There was a problem hiding this comment. Can we move this loop up one level, as in https://github.com/apache/iceberg/pull/14559/files#r2513719871? This avoids repeating it. |
||
| for (WriteResult result : resultList) { | ||
| Preconditions.checkState( | ||
| result.referencedDataFiles().length == 0, | ||
| "Should have no referenced data files for append."); | ||
| Arrays.stream(result.dataFiles()).forEach(appendFiles::appendFile); | ||
| } | ||
| } | ||
| String description = "append"; | ||
|
|
||
| // fail all commits as really its only one | ||
| commitOperation( | ||
| table, | ||
| branch, | ||
| appendFiles, | ||
| summary, | ||
| description, | ||
| newFlinkJobId, | ||
| operatorId, | ||
| pendingResults.lastKey()); | ||
| } else { | ||
| for (Map.Entry<Long, List<WriteResult>> e : pendingResults.entrySet()) { | ||
| long checkpointId = e.getKey(); | ||
| List<WriteResult> writeResults = e.getValue(); | ||
|
|
||
| RowDelta rowDelta = table.newRowDelta().scanManifestsWith(workerPool); | ||
| for (WriteResult result : writeResults) { | ||
| // Row delta validations are not needed for streaming changes that write equality deletes. | ||
| // Equality deletes are applied to data in all previous sequence numbers, so retries may | ||
| // push deletes further in the future, but do not affect correctness. Position deletes | ||
| // committed to the table in this path are used only to delete rows from data files that | ||
| // are | ||
| // being added in this commit. There is no way for data files added along with the delete | ||
| // files to be concurrently removed, so there is no need to validate the files referenced | ||
| // by | ||
| // the position delete files that are being committed. | ||
| Arrays.stream(result.dataFiles()).forEach(rowDelta::addRows); | ||
| Arrays.stream(result.deleteFiles()).forEach(rowDelta::addDeletes); | ||
| } | ||
|
|
||
| // Every Flink checkpoint contains a set of independent changes which can be committed | ||
| // together. While it is technically feasible to combine append-only data across | ||
| // checkpoints, | ||
| // for the sake of simplicity, we do not implement this (premature) optimization. Multiple | ||
| // pending checkpoints here are very rare to occur, i.e. only with very short checkpoint | ||
| // intervals or when concurrent checkpointing is enabled. | ||
| commitOperation( | ||
| table, branch, rowDelta, summary, "rowDelta", newFlinkJobId, operatorId, checkpointId); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
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.
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.
Could we change this comment to describe correctly why we do this?