Skip to content

Implement Iceberg Table Metadata Driver transform - #39883

Open
jrmccluskey wants to merge 9 commits into
apache:masterfrom
jrmccluskey:feat/iceberg-side-input-table-cache-pr2
Open

Implement Iceberg Table Metadata Driver transform#39883
jrmccluskey wants to merge 9 commits into
apache:masterfrom
jrmccluskey:feat/iceberg-side-input-table-cache-pr2

Conversation

@jrmccluskey

Copy link
Copy Markdown
Contributor

Implements the Iceberg table metadata query transform and materializes the table metadata to be used downstream as a side input.

Part of #39723


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.

@github-actions

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!

import org.joda.time.Instant;

/**
* A driver transform that extracts table identifiers from incoming {@link Row}s, deduplicates them

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.

Cloud you clarify in the docs how this helper will be used, specially given that this seem to be providing a sample of tables ?

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.

As-written this effectively acts as a maximum cache size, "sample" is maybe not the right word unless you get more tables than the configured maximum. My lack of experience with Iceberg kind of becomes a problem here, I'm not sure what a "typical" workload looks like in terms of the number of tables being operated on. We could do away with this parameter or make it uncapped by default if that makes more sense

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 was just using the wording from the existing comment :)

I think if we are trying to cache maximum possible for efficiency, but rest will still work (less efficiently), it makes sense. In general, lets's expand more about downstream use-case here to clarify what it's intended for.

@Test
public void testWindowedDeduplication() {
Catalog catalog = getCatalog();
TableIdentifier table1 = TableIdentifier.of("default", "t1");

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 expect to test same table ID being returned by multiple windows 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.

This is what I get for leaning on Gemini to produce some unit tests. The name is misleading, it's really testing that we deduplicate the target table IDs when we build the spec (since there are 50 elements referring to t1 and 50 elements referring to t2, we deduplicate that to single references to the two tables.) I'll rename this, the window doesn't really matter

public void processElement(
@Element String tableIdString, OutputReceiver<KV<String, SerializableTableSpec>> out) {
TableIdentifier tableId = IcebergUtils.parseTableIdentifier(tableIdString);
Table table = catalogConfig.catalog().loadTable(tableId);

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.

Note that a bad table string here will cause the whole bundle to fail. Is this intended or should we skip bad records and try other elements ?

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.

We could set up some sort of DLQ here. As best I can tell the current iceberg implementation uses the same approach of outright failing on bad table identifiers though

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.

Going back to the original design doc, Ahmed had suggested catching NoSuchTableExceptions gracefully since the lack of metadata tells the sink to try creating the table. I've added that logic, which should avoid failing whole bundles when we get an expected exception.

@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.34%. Comparing base (66ecc31) to head (11fffa9).
⚠️ Report is 27 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master   #39883      +/-   ##
============================================
+ Coverage     57.22%   58.34%   +1.12%     
- Complexity     3661    13472    +9811     
============================================
  Files          1195     2567    +1372     
  Lines        193411   268606   +75195     
  Branches       3831    11030    +7199     
============================================
+ Hits         110670   156717   +46047     
- Misses        79171   105952   +26781     
- Partials       3570     5937    +2367     
Flag Coverage Δ
java 64.45% <ø> (-7.75%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@bvolpato bvolpato 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.

LGTM. One nonblocking question inline about streaming input scope.

.apply("ExtractTableIds", ParDo.of(new ExtractTableIdsDoFn(getDynamicDestinations())))
.setCoder(StringUtf8Coder.of());

PCollection<String> distinctTableIds = tableIds.apply("DistinctTableIds", Distinct.create());

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.

nonblocking: is this meant to support default streaming inputs? Distinct rejects an unbounded GlobalWindow with default trigger. Iceberg write accepts that input shape before its own triggering. fine if this is batch/pre-windowed only, but worth making boundary explicit.

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.

Probably worth deduplicating a different way then, don't want to add an extra restriction here

@jrmccluskey

Copy link
Copy Markdown
Contributor Author

CC: @ahmedabu98

Currently exploring if moving to another method of deduplication makes sense, the original design doc specifically called out using Distinct but also wants unbounded global window streaming support.

@jrmccluskey

Copy link
Copy Markdown
Contributor Author

@chamikaramj @ahmedabu98 this should be ready for another review pass!

@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!

}

PCollection<String> distinctTableIds =
triggeredTableIds.apply("DistinctTableIds", Distinct.create());

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.

Deduplicate transform is generally a better alternative for streaming mode. it also has a .withDuration(refreshInterval) method that aligns well 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.

Looking at Deduplicate it's a stateful DoFn, which the initial problem statement and design scope seemed insistent on avoiding (mainly for the potential bottleneck from shuffles.) Is there a particular reason it's preferred for streaming workloads?

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 I feel silly, adding a schema evolution test case actually found that Distinct didn't re-emit the pane in an unbounded streaming context. That explains it.

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.

Yeah Distinct only emits once per window, so more batch-like.
AFAICT Deduplicate emits once and stores the value in state. It won't emit the value again while it's in state. After refreshInterval time passes, it clears the value and allows a new encounter to be emitted once again.

Deduplicate is a stateful DoFn, which the initial problem statement and design scope seemed insistent on avoiding

That's a good point cuz Deduplicate will still have one thread per tableId.
I think we can relieve some of the pressure here by having the upstream (pre-bottleneck) ExtractTableIdsDoFn transform do some local filtering. It can keep track of when tableIds where last emitted (e.g. Map<String, Instant>) and only output a tableId if it's older than refreshInterval / 2. Doesn't have to be exact or durable, if duplicates still get through then stateful Deduplicate will take care of them.

Also I think this bottleneck is a lot more gentle than the one the doc mentions. We're passing table strings through a side input instead of full data rows through the main path.

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 the more I thought about it the less I was worried about the bottleneck in a streaming context since we're 1) passing pretty lightweight objects and 2) anticipate relatively small bundle sizes and worker counts for streaming workloads. If it was the batch patch I would be more concerned since that's the use-case where the potential to DDOS during queries is high

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.

We can leave it as a future improvement if we notice it's affecting throughput. I don't think it'll break update compatibility

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 that shouldn't impact coders or graph shape, we can play with that later.

Comment on lines +227 to +229
return cachedTableIds
.apply("PollTableMetadata", ParDo.of(new CatalogPollingDoFn(getCatalogConfig())))
.setCoder(KvCoder.of(StringUtf8Coder.of(), SerializableTableSpec.getCoder()));

@ahmedabu98 ahmedabu98 Sep 1, 2026

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 are we going to control parallelism? Ideally we'd pass tableIds to only a few tables* so they can do sequential loadTable calls.

*this can be configurable

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'm not sure I'm following the ask here. Would we want to emit batches of table metadata downstream in that context?

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.

Sorry lemme clarify. I think as it stands, each tableId (after deduplication) can go to a separate worker. If we're writing to many tables, we can end up with many concurrent loadTable calls (one for each table).

Was wondering if it makes sense to reshuffle these table string outputs to a fixed N workers so that we only have at most N concurrent loadTable calls. If there's many tables, the workers can call loadTable on each one sequentially

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.

Reshuffle via random key with a fixed number of buckets would do that, I'll see how that looks.

Comment on lines +309 to +310
@Test
public void testMetadataRefreshedAcrossIntervals() {

@ahmedabu98 ahmedabu98 Sep 2, 2026

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 looks good, but can we please add another similar one consuming the table specifically as a side input? Should also assert the update happened

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 was a good call, adding that test variation found a number of problems with streaming cases and the triggering setup.

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.

4 participants