peter-toth commented on code in PR #58340:
URL: https://github.com/apache/spark/pull/58340#discussion_r3879483408
##########
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:
**Finding 6.** @LuciferYang, answering your question directly: no, I would
not put a qualifier on "a superset of their rows".
That clause is not decoration, it is the soundness argument. The merge is
only a rewrite because a projection and a filter above the merged scan can
recover each original scan's result, and that needs the merged scan to still
contain every row the narrow scan had. A source that can lose rows when a
column joins the projection does not have the property. A qualifier admitting
that would mean "declaring this capability permits Spark to change your
results", and nothing downstream could rely on the premise again - which is the
risk @dongjoon-hyun's comment is about.
Two facts have moved since you decided to keep CSV and JSON in, both
measured on this head, both in my reply on the migration-guide thread:
`PERMISSIVE` also changes (with `columnNameOfCorruptRecord` in the schema,
`count(_corrupt_record)` goes 0 to 1 on csv and json), and `FAILFAST` also
changes (a short CSV row makes the merged query throw where the unmerged one
returned rows). So this is not one unusual mode. It reaches the default mode,
and it can turn a working query into an error.
So I would take @dongjoon-hyun's shape, and one step further:
- `FileTable.CAPABILITIES` goes back to `EnumSet.of(BATCH_READ,
BATCH_WRITE)`, with a `CAPABILITIES_WITH_SCAN_MERGING` beside it and a
`protected def supportsScanMerging: Boolean = false` seam choosing between them
(see my comment on the new scaladoc).
- `ParquetTable`, `OrcTable`, `TextTable` and `AvroTable` override it to
`true`. `CSVTable` and `JsonTable` do not.
What that buys: the javadoc stays true of every source that declares the
capability; no third-party `FileTable` subclass is opted into a contract its
author never saw; and the migration-guide entry can go away, because no
user-visible result change is left.
On your objection that a seventh format would then silently not participate
- I think that is the right way round. A new format that does not merge is a
missed optimization, visible as a slower query. A new format that merges while
its parser is projection-sensitive is a wrong answer, visible as nothing. The
default should fail in the cheap direction, and the four `supportsScanMerging =
true` overrides sit next to `formatName` and `fallbackFileFormat`, which a new
format has to fill in anyway.
The honest counter-argument, because it is a real one: this leaves V2 CSV
and JSON merging less than V1 for column-only-differing scans, part of the gap
this PR set out to close. My read is that V1's behaviour here is a bug rather
than a target, since it makes one subquery's result depend on what a sibling
subquery projects, and matching a bug is worse than a fourth documented gap. If
you would rather fix V1 first and align V2 to it after, as you offered above,
that order works for me too. What I would avoid is weakening the capability's
contract so that the current shape becomes in-spec.
--
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]
