Skip to content

Updates the Delta Lake source to support reading bounded change data - #39426

Open
chamikaramj wants to merge 2 commits into
apache:masterfrom
chamikaramj:delta_lake_cdc
Open

Updates the Delta Lake source to support reading bounded change data#39426
chamikaramj wants to merge 2 commits into
apache:masterfrom
chamikaramj:delta_lake_cdc

Conversation

@chamikaramj

@chamikaramj chamikaramj commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

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:

  • Mention the appropriate issue in your description (for example: 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, comment fixes #<ISSUE NUMBER> instead.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

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)

Build python source distribution and wheels
Python tests
Java tests
Go tests

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@chamikaramj
chamikaramj marked this pull request as ready for review July 25, 2026 19:21
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@github-actions

Copy link
Copy Markdown
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@chamikaramj

Copy link
Copy Markdown
Contributor Author

assign set of reviewers

@github-actions

Copy link
Copy Markdown
Contributor

Assigning reviewers:

R: @Abacn for label java.

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).


@Override
public String toString() {
return "DeltaCDCReadTask{"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: prefer String.format

}
}

return ProcessContinuation.stop();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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:");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +247 to +249
if (getStartVersion() == null && getStartTimestamp() == null) {
throw new IllegalArgumentException("Either startVersion or startTimestamp must be set.");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IcebergIO default is we read from the beginning for batch and from the very end for streaming

Comment on lines +253 to +255
if (getEndVersion() != null && getEndTimestamp() != null) {
throw new IllegalArgumentException("Cannot set both endVersion and endTimestamp.");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above. If no end is specified, we can just stop at the very latest change

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: unnecessary assignment to another variable?

Comment on lines +285 to +286
final List<Row> cdcRows = new ArrayList<>();
final List<Row> addRows = new ArrayList<>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +266 to +268
.add("_change_type", StringType.STRING, false)
.add("_commit_version", LongType.LONG, false)
.add("_commit_timestamp", TimestampType.TIMESTAMP, false);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have these field names be final static variables at the top?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What values can _change_type take?

Also is _commit_version increasingly monotonic?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need ProcessContinuation here at all, since we never call resume() ?

Comment on lines +239 to +245
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();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If schemas are equal we can just return the original row

Comment on lines +1005 to +1015
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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +960 to +967
// 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));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add support for reading bounded CDC data

3 participants