DanielLeens commented on PR #11516: URL: https://github.com/apache/seatunnel/pull/11516#issuecomment-6190328942
Thanks for the extra review, @SEZ9, and thanks @fatmanverse for your patience. CI correction first: in my previous round I reported the required `Build` check as failing on this head (a Maven wrapper 404 in the fork run). That fact has changed. For head `10e70d2257a0` the apache-side `Build` check now shows `success`, so there is no CI blocker left from my side. On SEZ9's findings, I re-checked the cited code at this head and the evidence holds: - Issue 1 (Hive multi-table behavior change): confirmed. `MultipleTableHiveSourceReader.java:83` calls `readStrategy.read(split.getFilePath(), split.getTableId(), output)`, and before this PR `ParquetReadStrategy` built `new FileSourceSplit(path)`, which hard-codes `tableId = null` (`FileSourceSplit.java:38-42`). So Hive Parquet rows now carry the catalog table path instead of null. This aligns Parquet with the other format strategies, so I still consider it the right direction and it does not change my "no source-level blocker" verdict, but I agree with +1 to updating the PR description (it currently reads as Spark-only) and ideally adding a multi-table Parquet assertion on the emitted tableId. - Issues 2 to 4: reasonable and non-blocking. Dropping the guard or applying it consistently, asserting `""` in `testParquetRead1`, and rewording the guard comment to name `FileSourceSplit(String)` as the origin of the null would all make the change easier to maintain. So the technical state from my side stays "cleared": the remaining items are SEZ9's non-blocking follow-ups plus the maintainer approval gate. Once a new commit lands I will re-check it. -- 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]
