LuciferYang commented on PR #58340: URL: https://github.com/apache/spark/pull/58340#issuecomment-5448854949
Thanks for the review. Eight of the ten findings are addressed; the two on the `FileTable` comment have replies inline instead. Fixed as suggested: - The `nestedPruning=false` branch was vacuous, as you said. That arm now asserts `subqueryCounts`, `(1, 1)` merged against `(2, 0)` declined, and `distinctScans == 2` stays only on the `nestedPruning=true` arm where the two `readSchema`s differ. The comment says why the old assertion could not tell the two apart. - `assertNoPlaceholderRelation` and its four call sites are gone. Your reasoning holds: `DataSourceV2Strategy`'s only batch-read case matches `DataSourceV2ScanRelation`, so a leaked bare relation fails planning inside `checkAnswer` before the helper runs. - `v2Scans` no longer exists twice. It moved with `distinctScans` into a package-private `V2ScanMergingTestHelper` that both suites mix in. I left `PlanMergingSuite`'s inlined subquery counting alone to keep this diff to the duplication you pointed at. - The redundant executions are gone: the parse-mode test binds the result once, and the differing-data-filters test writes its data outside the flag loop, matching the nested-fields test. - `connector/avro` has coverage now. A test in `AvroV2Suite` asserts `AvroTable` carries the capability and that two scans differing only in their projected columns fuse into one reading the union. - The migration guide says "for these shapes" where it claimed to match V1, so the partition-filter gap the suite pins does not read as closed. Adjusted rather than taken as written: - On the shared `CAPABILITIES`: `FileTable` now has a class-level scaladoc stating the inherited contract for subclass authors, which was your fallback suggestion. I kept the single declaration rather than repeating it in the six built-in tables, because a seventh format added later would then silently not participate, and that drift is harder to notice than the inherited opt-in. - On the parse modes: I measured all three rather than extending the guide to cover them. `PERMISSIVE` returns the same rows merged and unmerged, because the parser keeps the fields it did parse, and `FAILFAST` throws either way, because the merged scan reads the union of the two column sets and the scan reading the malformed column already throws on its own. The guide now records that neither mode changes instead of widening the warning, and both are assertions in the suite. Numbers are in the reply on the `FileTable` comment. `FileSourceV2PlanMergingSuite`, `DSv2PlanMergingSuite` and `PlanMergingSuite` pass, 30 tests, along with the new Avro test and scalastyle on `sql` and `connector/avro`. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
