Repository navigation
Fix correctness issue in JsonToRow, RowJsonDeserializer and RowJsonSerializer - #40769
sjvanrossum wants to merge 4 commits into
Conversation
Adds an EnumerationType field to RowJsonTest.ValueTests.makeLogicalTypeTestCase() to verify serialization, deserialization, and round-tripping of logical types whose input type (EnumerationType.Value) differs from their base type (INT32).
RowJson only special-cased SQL date/time logical types by identifier and fell through to the underlying base FieldType for all other logical types without converting between the logical input type (T) and base type (BaseT), causing ClassCastExceptions on serialize and deserialize.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review Summary
This PR fixes two correctness bugs: missing toInputType/toBaseType conversions for non-SQL logical types in RowJson serialization/deserialization, and hardcoded dead-letter schema field names in JsonToRowWithErrFn when custom line or error field names are configured. Both fixes are clean, accurate, and accompanied by unit test updates.
🔍 General Feedback
- The
RowJsonlogical type conversion properly delegates base value extraction/serialization and applieslogicalType.toInputTypeandlogicalType.toBaseTypesymmetrically. JsonToRowWithErrFn.getErrorSchema()cleanly resolves the configured field names while reusing the static schema instances for default configurations.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Verifies that JsonToRow.withExtendedErrorInfo() propagates custom lineField and errorField names to both the dead-letter PCollection schema and the emitted error Rows.
7cbfde5 to
454a550
Compare
|
Assigning reviewers: R: @damccorm for label java. This pull request likely touches a core component ("core" label). Please review with scrutiny. 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 #40769 +/- ##
===========================================
- Coverage 80.04% 0 -80.05%
===========================================
Files 490 0 -490
Lines 82916 0 -82916
===========================================
- Hits 66373 0 -66373
+ Misses 16543 0 -16543
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`JsonToRowWithErrFn.expand()` hardcoded the default 'line' and 'err' field names when registering the dead-letter PCollection schema instead of using `lineField()` and `errorField()`, causing a schema mismatch when custom field names were configured.
454a550 to
0c47006
Compare
|
Run Java Examples_Dataflow PreCommit |
RowJsononly special-cased SQL date/time logical types by identifier and fell through to the underlying baseFieldTypefor all other logical types without converting between the logical input type (T) and base type (BaseT), causingClassCastExceptions on serialize and deserialize.JsonToRowWithErrFn.expand()hardcoded the defaultlineanderrfield names when registering the dead-letterPCollectionschema instead of usinglineField()anderrorField(), causing a schema mismatch when custom field names were configured.