Repository navigation
[#18690] Port Iceberg IO tests off deprecated DoFnTester - #40909
Supersaiyan101 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
📋 Review Summary
This PR cleanly migrates four test classes in sdks/java/io/iceberg off the deprecated DoFnTester by directly invoking DoFn lifecycle and element processing methods with lightweight OutputReceiver and MultiOutputReceiver implementations.
🔍 General Feedback
- In
LocalResolveDoFnTestandResolveChangesTest, the customOutputReceiverimplementation fails to propagateValueKindwhen elements are emitted viaOutputBuilder(calling the 4-argValueInSingleWindow.ofoverload which hardcodesValueKind.INSERT). This is a critical bug that will cause CDC update/delete assertions (UPDATE_BEFORE,UPDATE_AFTER,DELETE) to fail. - Since
OutputReceiveralready providesdefaultimplementations foroutputandoutputWithTimestampthat delegate tobuilder(output), overriding those methods in the anonymousOutputReceiverclasses is redundant.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
| .setReceiver( | ||
| wv -> | ||
| outputs.add( | ||
| ValueInSingleWindow.of( |
There was a problem hiding this comment.
🔴 Critical: LocalResolveDoFn emits CDC records using out.builder(row).setValueKind(kind).output(), and the tests assert on value.getValueKind() (via kindAndProjectedRow). However, the 4-argument ValueInSingleWindow.of(value, timestamp, window, paneInfo) overload hardcodes ValueKind.INSERT, discarding wv.getValueKind(). As a result, all emitted records will have ValueKind.INSERT instead of UPDATE_BEFORE, UPDATE_AFTER, or DELETE, causing the tests in this class to fail.
Use the 9-argument ValueInSingleWindow.of(...) overload and pass wv.getValueKind() (and wv.causedByDrain()). You can also remove the redundant output and outputWithTimestamp overrides since OutputReceiver already provides default methods that delegate to builder(...).
| .setReceiver( | ||
| wv -> | ||
| outputs.add( | ||
| ValueInSingleWindow.of( |
There was a problem hiding this comment.
🔴 Critical: Similar to LocalResolveDoFnTest, ResolveChanges sets the CDC kind via .setValueKind(kind) on the OutputBuilder, and kindAndData inspects value.getValueKind(). Calling the 4-argument ValueInSingleWindow.of(...) overload drops wv.getValueKind() and defaults to ValueKind.INSERT, which breaks updatePairAndExtraRowsPreserveKindsAndTimestamp.
Pass wv.getValueKind() using the 9-argument ValueInSingleWindow.of(wv.getValue(), wv.getTimestamp(), wv.getWindows().iterator().next(), wv.getPaneInfo(), wv.getRecordId(), wv.getRecordOffset(), wv.causedByDrain(), wv.getOpenTelemetryContext(), wv.getValueKind()) overload (and optionally drop the redundant output/outputWithTimestamp overrides).
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public <T> java.util.List<T> getOutput(TupleTag<T> tag) { |
There was a problem hiding this comment.
🟢 Nit: Prefer importing java.util.Collections and java.util.List at the top of the file rather than using fully-qualified names here and on line 67.
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR cleanly migrates four test classes in sdks/java/io/iceberg off the deprecated DoFnTester by directly invoking the DoFn lifecycle and @ProcessElement methods using TestOutputReceiver and WindowedValues.builder(). All review comments from the initial revision regarding preserving ValueKind on ValueInSingleWindow outputs and import cleanup have been properly addressed.
🔍 General Feedback
- The replacements in
BundleLifterTest,ApplyWatermarkColumnTest,LocalResolveDoFnTest, andResolveChangesTestaccurately preserve the original test semantics (including bundle lifecycle, output tags, timestamps, and CDCValueKindmetadata).
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Assigning reviewers: R: @chamikaramj for label iceberg. 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). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40909 +/- ##
=======================================
Coverage 79.83% 79.84%
=======================================
Files 494 494
Lines 83575 83525 -50
=======================================
- Hits 66722 66690 -32
+ Misses 16853 16835 -18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This PR addresses #18690 by porting all remaining tests in
sdks/java/io/iceberg/off of the deprecatedDoFnTester:BundleLifterTest: replacedDoFnTesterwith direct invocation of DoFn lifecycle methods (startBundle,processElement,finishBundle) along withTestOutputReceiverandMultiOutputReceiver.ApplyWatermarkColumnTest: replacedDoFnTesterwith direct invocation offn.process(row, receiver)to verify expected exception throwing.LocalResolveDoFnTest&ResolveChangesTest: replacedDoFnTesterwith direct DoFn invocation collectingValueInSingleWindow<Row>with timestamp and pane information.All changed tests were verified and formatted with
./gradlew :sdks:java:io:iceberg:spotlessApply.