Skip to content

[Iceberg CDC] Finish wiring CDC source together and add external API - #39600

Merged
ahmedabu98 merged 5 commits into
apache:masterfrom
ahmedabu98:iceberg-full-cdc-read
Aug 5, 2026
Merged

[Iceberg CDC] Finish wiring CDC source together and add external API#39600
ahmedabu98 merged 5 commits into
apache:masterfrom
ahmedabu98:iceberg-full-cdc-read

Conversation

@ahmedabu98

@ahmedabu98 ahmedabu98 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Finalizes the IcebergIO CDC streaming source. Introduces top-level IncrementalChangelogSource to replace IncrementalAppendSource.

Reads a table's changelog one snapshot at a time and emits net per-snapshot changes as CDC rows. Bi-directional groups too large to resolve in memory are windowed per snapshot (SnapshotWindowFn), grouped by primary key, and reconciled by ResolveChanges, while small groups continue to resolve locally (LocalResolveDoFn) without a shuffle.

Adds user-facing options for CDC metadata columns, a custom watermark column with its time unit, and a maximum snapshot discovery delay.

Removes the now-unused IncrementalScanSource, ReadFromTasks, and WatchForSnapshots.

Fixes #38831


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.

@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Assigning reviewers:

R: @chamikaramj 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).

@chamikaramj chamikaramj left a comment

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.

Thanks!


// process one snapshot at a time and produce batches of changelog scan tasks.
// tasks are emitted to three outputs:
// 1. unidirectional tasks: we know these won't have any updates

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.

How about deletes ?
Did you mean "these only contain inserts" ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

"unidirectional tasks" refers to a group of tasks where we are certain there is no insert-delete collision.

For example, it may contain ONLY inserts, or ONLY deletes. or it may contain inserts and deletes that don't overlap in PK

PCollection<Row> smallBidirectionalCdcRows =
changelogTasks
.get(SMALL_BIDIRECTIONAL_TASKS)
.apply("Redistribute Small Bidirectional Changes", Redistribute.arbitrarily())

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.

Seems like we have a shuffle here ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah but this is before we apply any ValueKind metadata. Same with the CoGroupByKey in the large bi-directional path -- it's before any ValueKind metadata gets added

Window<KV<CdcRowDescriptor, Row>> keyedWindowing =
Window.<KV<CdcRowDescriptor, Row>>into(new SnapshotWindowFn())
.triggering(AfterWatermark.pastEndOfWindow())
.withAllowedLateness(Duration.ZERO)

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.

Is dropping late data the correct approach here ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The watermark here is actually controlled by us (in WatchForSnapshotsSdf). We only advance it past a snapshot's timestamp after emitting that snapshot, so in the normal flow there's no "late" data. Timestamps represent snapshot commit time

There was one exception I just fixed. On empty polls we bump the watermark to now() - pollInterval so downstream windows don't stall while the table is quiet. But if snapshots are committed and only discovered much later (e.g. a catalog outage), that bump could run ahead of their commit times, and they'd arrive late and get silently dropped. Fixed this edge case by setting output timestamp = max(commitTs, currentWatermark).

KeyedPCollectionTuple.of(INSERTS, keyedInserts)
.and(DELETES, keyedDeletes)
.apply("CoGroupBy Primary Key", CoGroupByKey.create())
.apply("Resolve Delete-Insert Pairs", ParDo.of(new ResolveChanges(scanConfig)))

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 suspect this is something we can do at the framework level later right ? (Without having to do this kind of optimizations per source)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This logic is written just for Iceberg actually. Iceberg doesn't have any native concept of an "update", so we (the engine) have to figure it out ourselves

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But it can be mostly re-used if we encounter another source with similar behavior

}

@Override
protected int nonPkHash(Row element) {

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.

How expensive is this ? Seems like we are hashing pretty much all fields here of all elements ? (not a blocker)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well we need to compare all fields to correctly determine if it's an update. This is only done for records that fall in the overlap range. Other records skip this entirely

}

@Override
protected boolean nonPkEquals(Row delete, Row insert) {

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.

Ditto (might be good to do a perf analysis after the PR is submitted)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See comment above. We only do this equality comparison on hash collision, so it's even more rare.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Perf analysis could be warranted though

* require a row-lineage table. The changelog metadata columns come from the emitted change kind
* and snapshot context and are appended when final Beam rows are emitted.
*
* <p>This option is only valid {@link #withCdc()}.

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.

Let's fail if the incorrect combination is set.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does in IcebergScanConfig#validate

@ahmedabu98
ahmedabu98 requested a review from chamikaramj August 5, 2026 17:25

@chamikaramj chamikaramj left a comment

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.

Thanks!

if (ts.isBefore(watermark.currentWatermark())) {
// The watermark already moved past this snapshot's commit time (e.g. the idle bump ran
// ahead of a slow discovery). Use the current watermark so the snapshot is not dropped
ts = watermark.currentWatermark();

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.

Probably there still a race (extremely rare) between we setting this here and runner firing for the current watermark, right ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, IIUC the watermark only advances when the bundle commits, along with the emmitted elements. and those same in-flight elements hold back the watermark until they arrive downstream, so a downstream window can't fire until they all arrive

abstract @Nullable String getWatermarkColumnTimeUnit();

@SchemaFieldDescription(
"Maximum expected snapshot discovery delay in seconds. While idle, the source may advance "

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.

Did you intend to remove this knob ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah we no longer need it after the fix mentioned in #39600 (comment)

Only had it to allow customers to control what we would consider "late". but now even if the catalog is slow/down, we wouldn't consider the missed snapshots as late. We just weave them in and attach current watermark as timestamp

@ahmedabu98
ahmedabu98 merged commit c60b021 into apache:master Aug 5, 2026
24 checks passed
rwiggles pushed a commit to rwiggles/beam-rwiggles that referenced this pull request Aug 5, 2026
…pache#39600)

* wrap up

* trigger ITs and add to CHANGES.md

* fix late snapshot edge case

* update resolution optimization

---------

Co-authored-by: Ahmed Abualsaud <ahmedabualsaud@MacBook-Pro-2.local>
rwiggles pushed a commit to rwiggles/beam-rwiggles that referenced this pull request Aug 6, 2026
…pache#39600)

* wrap up

* trigger ITs and add to CHANGES.md

* fix late snapshot edge case

* update resolution optimization

---------

Co-authored-by: Ahmed Abualsaud <ahmedabualsaud@MacBook-Pro-2.local>
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.

[Task]: Implement a full Iceberg CDC incremental source

2 participants