Updates the Delta Lake source to support reading bounded change data - #39426
Updates the Delta Lake source to support reading bounded change data#39426chamikaramj wants to merge 2 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
ed28244 to
82ab11e
Compare
|
assign set of reviewers |
|
Assigning reviewers: R: @Abacn for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
|
||
| @Override | ||
| public String toString() { | ||
| return "DeltaCDCReadTask{" |
| } | ||
| } | ||
|
|
||
| return ProcessContinuation.stop(); |
There was a problem hiding this comment.
This is the only place where DoFn exits. if there is error throws, will it retry the whole element? In other words, would bounded SDF just resume from a claimed restriction even though previously crashed in the middle of a DoFn.process invocation?
There was a problem hiding this comment.
I wonder if we need ProcessContinuation here at all, since we never call resume() ?
|
|
||
| private static StructType appendCDFColumns(StructType schema) { | ||
| return schema | ||
| .add("_change_type", StringType.STRING, false) |
There was a problem hiding this comment.
nit: consider delcare these field name as constants. There are used in multiple places
|
|
||
| while (logicalBatches.hasNext()) { | ||
| FilteredColumnarBatch batch = logicalBatches.next(); | ||
| ColumnarBatch logicalBatch = batch.getData(); |
There was a problem hiding this comment.
(based on AI comment): batch.getData() return raw data, ignoring selectionVector and causing filtered-out or deleted rows to be iterated over and emitted into the output PCollection.
Suggestion, call batch. getRows which handles selectionVector
while (logicalBatches.hasNext()) {
FilteredColumnarBatch batch = logicalBatches.next();
if (!task.isCDC()) {
ColumnarBatch logicalBatch = appendConstantCDFColumns(
currentEngine, batch.getData(), task.getVersion(), task.getTimestamp());
batch = new FilteredColumnarBatch(logicalBatch, batch.getSelectionVector());
}
try (CloseableIterator<io.delta.kernel.data.Row> logicalRows = batch.getRows()) { // <-- Uses FilteredColumnarBatch.getRows()
while (logicalRows.hasNext()) {
io.delta.kernel.data.Row deltaRow = logicalRows.next();
...
}
}
}|
|
||
| writePipeline.run().waitUntilFinish(); | ||
|
|
||
| System.out.println("FILES IN TABLE DIR:"); |
There was a problem hiding this comment.
Not related to the change. But running the test realize there are System.out.println and System.err.println debugging leftovers. Consider removing them or using slf4j LOGGER.
| } | ||
| } | ||
|
|
||
| private byte[] writeParquetFile(File file, Schema schema, java.util.List<Row> rows) |
There was a problem hiding this comment.
| if (getStartVersion() == null && getStartTimestamp() == null) { | ||
| throw new IllegalArgumentException("Either startVersion or startTimestamp must be set."); | ||
| } |
There was a problem hiding this comment.
If there's no start version specified, we can have a default starting strategy. Either start at the very beginning, or at the very latest change.
Not blocking though, can add this in a future change
There was a problem hiding this comment.
The IcebergIO default is we read from the beginning for batch and from the very end for streaming
| if (getEndVersion() != null && getEndTimestamp() != null) { | ||
| throw new IllegalArgumentException("Cannot set both endVersion and endTimestamp."); | ||
| } |
There was a problem hiding this comment.
Same as above. If no end is specified, we can just stop at the very latest change
There was a problem hiding this comment.
I think your logic in the Create DoFn already handles this
| boolean hasCDC = !info.cdcRows.isEmpty(); | ||
|
|
||
| List<Row> rowsToProcess = hasCDC ? info.cdcRows : info.addRows; | ||
| boolean isCDC = hasCDC; |
There was a problem hiding this comment.
nit: unnecessary assignment to another variable?
| final List<Row> cdcRows = new ArrayList<>(); | ||
| final List<Row> addRows = new ArrayList<>(); |
There was a problem hiding this comment.
Naming is throwing me off a little. So these aren't data rows right? Each row is metadata about a particular CDC or INSERT task?
| * files, converting rows to Beam Rows. | ||
| */ | ||
| @DoFn.BoundedPerElement | ||
| class DeltaCDCSourceDoFn extends DoFn<DeltaCDCReadTask, Row> { |
There was a problem hiding this comment.
This DoFn parallelizes at the file level right? I think this should be fine for batch, but note that it may not be scalable in DF streaming mode because the runner opens too many threads per VM, each thread processing a file and competing for memory leading to OOM.
| .add("_change_type", StringType.STRING, false) | ||
| .add("_commit_version", LongType.LONG, false) | ||
| .add("_commit_timestamp", TimestampType.TIMESTAMP, false); |
There was a problem hiding this comment.
Can we have these field names be final static variables at the top?
There was a problem hiding this comment.
What values can _change_type take?
Also is _commit_version increasingly monotonic?
There was a problem hiding this comment.
hmmm I see that delta uses update_preimage and update_postimage for UPDATE_BEFORE/AFTER. This might not play too well with Iceberg CDC sink but I think should be okay because we can rely on the ValueKind metadata instead
| } | ||
| } | ||
|
|
||
| return ProcessContinuation.stop(); |
There was a problem hiding this comment.
I wonder if we need ProcessContinuation here at all, since we never call resume() ?
| private static Row projectRow(Row row, Schema targetSchema) { | ||
| Row.Builder builder = Row.withSchema(targetSchema); | ||
| for (Schema.Field field : targetSchema.getFields()) { | ||
| builder.addValue(row.getValue(field.getName())); | ||
| } | ||
| return builder.build(); | ||
| } |
There was a problem hiding this comment.
If schemas are equal we can just return the original row
| private void writeCommit( | ||
| File logDir, | ||
| long version, | ||
| long timestamp, | ||
| @Nullable String addPath, | ||
| long addSize, | ||
| @Nullable String removePath, | ||
| @Nullable String cdcPath, | ||
| long cdcSize, | ||
| boolean writeMetadata) | ||
| throws IOException { |
There was a problem hiding this comment.
Can we commit using the DeltaLake API? to make sure it works against the actual API and get notified if a library upgrade breaks things
| // Test 1: Read changes between start version 0 and end version 2 | ||
| PCollection<Row> outputVersions = | ||
| readPipeline.apply( | ||
| "Read Changes Version Range", | ||
| DeltaIO.readChanges() | ||
| .from(tableDir.getAbsolutePath()) | ||
| .withStartVersion(0L) | ||
| .withEndVersion(2L)); |
There was a problem hiding this comment.
This test reads the whole range, so it can still pass if the range handling logic happened to be incorrect. We should have start version > 0 and/or end version < 2
Please see here for a short design description.
This fixes #39492.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.