Skip to content

[Iceberg] CDC config and writer - #39997

Open
ahmedabu98 wants to merge 7 commits into
apache:masterfrom
ahmedabu98:cdc-delta-writer-and-config
Open

[Iceberg] CDC config and writer#39997
ahmedabu98 wants to merge 7 commits into
apache:masterfrom
ahmedabu98:cdc-delta-writer-and-config

Conversation

@ahmedabu98

Copy link
Copy Markdown
Contributor

Adds the CDC sink's delta writer, which receives sorted rows (by PK, sequence number, change kind). The writer collapses changes and only writes one final state per key per window (an equality delete plus the surviving row). Because it collapses changes, it never needs to write a deletion vector.

Also adds the sink's CdcWriteConfig.

Need to merge #39981 first.

Part of #39979


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.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

The current CdcWriteConfig defaults/validation can produce invalid configs (notably around shards-per-partition defaulting) and there are a couple of robustness/determinism issues that should be addressed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR introduces core components for an Iceberg CDC sink in the Java SDK: a delta task writer that consumes sorted CDC records and collapses per-key changes into at most one equality delete plus one final row per window, and a corresponding CdcWriteConfig configuration object. It also adds targeted tests validating both the writer’s flush/partition-routing behavior and the config’s validation/serialization behavior.

Changes:

  • Added RecordDeltaTaskWriter to collapse sorted CDC streams into Iceberg row-delta writes (equality deletes + final rows) without DV/position deletes.
  • Added CdcWriteConfig (AutoValue) with validation for CDC sink options.
  • Added comprehensive unit tests for writer behavior and config validation/serialization.
File summaries
File Description
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/cdc/sink/RecordDeltaTaskWriter.java Adds the delta writer that collapses CDC changes per PK and writes equality deletes + final rows, including partition fanout support.
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/cdc/sink/CdcWriteConfig.java Adds sink configuration with defaults and validation for CDC-specific options.
sdks/java/io/iceberg/src/test/java/org/apache/beam/sdk/io/iceberg/cdc/sink/RecordDeltaTaskWriterTest.java Adds a flush “truth table” and partition-routing tests validating the writer’s semantics.
sdks/java/io/iceberg/src/test/java/org/apache/beam/sdk/io/iceberg/cdc/sink/CdcWriteConfigTest.java Adds tests covering defaults, validation rejection matrix, and Java serialization of the config.
Review details

Suppressed comments (3)

sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/cdc/sink/CdcWriteConfig.java:135

  • Given the intended semantics ("unset shards_per_partition resolves to num_shards"), setting shards_per_partition to a fixed default in builder() prevents that and can cause validate() to fail when num_shards is changed. With a resolved getter, this default should be removed so the value can remain unset.
  static Builder builder() {
    return new AutoValue_CdcWriteConfig.Builder()
        .setSequenceNumberColumn(DEFAULT_SEQUENCE_NUMBER_COLUMN)
        .setNumShards(DEFAULT_NUM_SHARDS)
        .setShardsPerPartition(DEFAULT_NUM_SHARDS)
        .setSorterMemoryMB(DEFAULT_SORTER_MEMORY_MB)
        .setUpsert(false)
        .setErrorHandling(false);

sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/cdc/sink/CdcWriteConfig.java:188

  • validate() will throw a NullPointerException if snapshot_properties contains a null key (due to key.startsWith(...)). Consider rejecting null keys explicitly so invalid user input fails with a clear IllegalArgumentException.
    @Nullable Map<String, String> snapshotProperties = getSnapshotProperties();
    if (snapshotProperties != null) {
      for (String key : snapshotProperties.keySet()) {
        checkArgument(
            !key.startsWith("beam.cdc."),
            "snapshot_properties key '%s' uses the reserved 'beam.cdc.' prefix; choose a "
                + "different key.",
            key);

sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/cdc/sink/CdcWriteConfig.java:216

  • After introducing a resolved getShardsPerPartition() method, the AutoValue property should be a nullable "override" (unset vs explicitly set). The builder currently only exposes setShardsPerPartition(int), which can't represent "unset"; adding an override setter keeps the external API while enabling correct defaulting behavior.
    abstract Builder setNumShards(int numShards);

    abstract Builder setShardsPerPartition(int shardsPerPartition);

  • Files reviewed: 4/4 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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.

2 participants