DanielLeens commented on PR #12009:
URL: https://github.com/apache/seatunnel/pull/12009#issuecomment-5476926514
Thanks for the very thorough parallel review, @SEZ9 — this is a great
complement to mine.
I went back and directly verified the one open factual question your review
raises (Issues 2 and 5): whether `AvroToRowConverter` (the read path) still has
any `toLowerCase()` on field names that could make the write-side fix
asymmetric. I pulled `AvroToRowConverter.java` at the PR head (`72d861a7`) and
there is no `toLowerCase()` anywhere in that file — every lookup is already
exact-case:
```java
if (record.getSchema().getField(fieldNames[i]) == null) { ... } //
AvroToRowConverter.java:83
values[i] = convertField(rowType.getFieldType(i),
record.get(fieldNames[i])); // AvroToRowConverter.java:87
```
So the read path was never part of the bug and is untouched by this PR,
which confirms your own read of the current code ("the fix is correct... as
written today") — I just wanted to close the loop on that specific factual
check rather than leave it as an open question.
On the rest of your findings: I agree Issues 1 and 3 (bind by `Schema.Field`
position instead of by name, so the write side and the generated schema can't
structurally drift) are good hardening for the future — today there's no bug
there since `recordSchema`/`schema` and the row type are built from the same
source in the same method call, but making that invariant structural rather
than implicit is a legitimate defensive improvement. Issues 4, 6, 7 and 8
(changelog note, null/nullable-union coverage, and `ARRAY<ROW>`/`MAP<_, ROW>`
nested-collection coverage) are all sensible, non-blocking test/doc
improvements that would strengthen this further.
None of these are correctness blockers for the fix itself — they're all
Medium/Low hardening and coverage suggestions on code paths that are provably
correct as written today, which matches your own severity ratings. My merge
recommendation from my earlier review stands: **Ready to merge after fixes**,
and the only actual blocker is still the pre-existing CI flakes unrelated to
this diff (`CoordinatorServiceTest` timeout in `seatunnel-engine-server`,
`CouchbaseIT` container-startup flake) — not any code issue. Happy to see the
position-binding and extra test-coverage suggestions land either in this PR or
as fast, low-risk follow-ups.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
