SEZ9 commented on PR #11516: URL: https://github.com/apache/seatunnel/pull/11516#issuecomment-6207444307
Thanks for the update. Here is where the previously raised points stand for head `10e70d2257a0`. **1. Hive multi-table Parquet behavior change (PR11516-F1)** — Before this PR `ParquetReadStrategy` built `new FileSourceSplit(path)`, which hard-codes `tableId = null`; with the fix, Hive Parquet rows on Zeta/Flink now carry the real split tableId, which changes what `MultiTableSinkWriter` sees for existing pipelines. I think this is the right direction since it aligns Parquet with the other format strategies, but two asks: - Please update the PR description (it currently reads as Spark-only) to call out this Hive multi-table change explicitly. - Please add a multi-table Parquet test asserting the emitted tableId. **2. Spark null-tableId guard (PR11516-F2)** — Non-blocking. Please either drop the guard in `InternalRowConverter.parcel` (the root-cause fix should make it unnecessary) or apply it consistently to `SeaTunnelRowConverter.parcel` and the `MultiTableManager` multi-table lookups so the defensive layer covers the same input everywhere. **3. Test coverage (PR11516-F3)** — Please extend `testParquetRead1` to assert the `""` id that `BaseFileSourceReader` actually passes, and add an assertion for the native-parquet fallback reader path. **4. Guard comment (PR11516-F4)** — Please reword the inline comment to name `FileSourceSplit(String)` as the origin of the null and state that `""` is a sentinel the multi-table routing cannot resolve. None of these are blockers from my side. Once a new commit lands with the description update and the test/comment changes, I'll re-check. <!-- streview-comment:1796 --> -- 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]
