LuciferYang commented on code in PR #58340:
URL: https://github.com/apache/spark/pull/58340#discussion_r3878157694
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/FileTable.scala:
##########
@@ -181,5 +181,9 @@ abstract class FileTable(
}
object FileTable {
- private val CAPABILITIES = util.EnumSet.of(BATCH_READ, BATCH_WRITE)
+ // A file table meets the determinism contract SCAN_MERGING requires:
`fileIndex` is a lazy val,
+ // so every scan built from this table lists the same files, and
`newScanBuilder` returns a fresh
+ // builder over `mergedOptions(options)`. The same options, pushed filters
and pruned columns
+ // therefore rebuild an equivalent scan.
+ private val CAPABILITIES = util.EnumSet.of(BATCH_READ, BATCH_WRITE,
SCAN_MERGING)
Review Comment:
Thanks, this is a real gap, and you have put it more precisely than the
suite's own comment does. Agreed on the mechanism: with `DROPMALFORMED` the
merged scan parses the union of the columns, so a record malformed only in the
other subquery's column is dropped for both, and the merged scan returns fewer
rows than the narrower scan did. That is not a superset.
On withholding the capability when `mode` is not `PERMISSIVE`, I measured
what that would buy first. Same query as the suite's, CSV and JSON, all three
modes, with `MergeSubplans` on and excluded:
| mode | merged | not merged |
|---|---|---|
| `PERMISSIVE` | `[10, 80]` | `[10, 80]` |
| `DROPMALFORMED` | `[8, 80]` | `[10, 80]` |
| `FAILFAST` | throws | throws |
The V1 path also returns `[8, 80]` under `DROPMALFORMED`; the suite asserts
it, and it is why the migration guide says the V2 path now matches V1. Gating
on the parse mode would leave the same query returning 10 on V2 and 8 on V1, so
it trades a violated javadoc premise for a result that depends on
`spark.sql.sources.useV1SourceList`. I would rather the two paths agree.
If the `[8, 80]` result is itself the thing to fix, that is a question about
both paths rather than about this capability: V1 returned 8 before this PR too,
and this PR does not touch it. I am happy to take that as a follow-up, either
as one change covering both paths or as a fix to V1 that this PR then aligns
to. If you would rather see V1 fixed first, I can hold this one until that
lands.
`PERMISSIVE` keeps the fields it did parse, so widening the parsed columns
changes nothing there. `FAILFAST` throws whether or not the scans merged,
because the merged scan reads the union of the two column sets and the scan
that reads `b` already throws on its own. Both are now assertions in the suite
rather than claims in a comment.
Your second suggestion is the one I would like to act on: the exception
belongs in the capability's own javadoc, not only in a test comment and a
migration-guide sentence. `TableCapability` is `@Evolving` public API and that
sentence arrived with #57360, so I would rather not reword it unilaterally.
cc @peter-toth, who wrote #57360 and the plan-merging work under it. Does
the "superset of their rows" sentence need a qualifier for sources whose
parsing depends on which columns are read, and would you rather that land here
or in a follow-up? I am happy either way.
--
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]
