peter-toth commented on code in PR #58340:
URL: https://github.com/apache/spark/pull/58340#discussion_r3892890241
##########
docs/sql-performance-tuning.md:
##########
@@ -384,7 +384,7 @@ In TPC-DS benchmark runs, enabling symmetric filter
propagation made `q9` and `q
<td><code>spark.sql.optimizer.mergeSubplans.filterPropagation.dsv2SymmetricFilterPropagation.enabled</code></td>
<td>false</td>
<td>
- When true, two DataSource V2 scans that pushed the same strictly
enforced filters but carry different best-effort (post-scan) filters can be
merged even when
<code>spark.sql.optimizer.mergeSubplans.filterPropagation.symmetricFilterPropagation.enabled</code>
is false. In this case widening cannot change the set of rows the scan is
required to return, as the strict filters are re-pushed unchanged and the
enclosing <code>Filter</code> re-checks the rest above the scan. This applies
only to V2 sources that opt in to scan merging with the
<code>SCAN_MERGING</code> table capability; no built-in source does.
+ When true, two DataSource V2 scans that pushed the same strictly
enforced filters but carry different best-effort (post-scan) filters can be
merged even when
<code>spark.sql.optimizer.mergeSubplans.filterPropagation.symmetricFilterPropagation.enabled</code>
is false. In this case widening cannot change the set of rows the scan is
required to return, as the strict filters are re-pushed unchanged and the
enclosing <code>Filter</code> re-checks the rest above the scan. This applies
only to V2 sources that opt in to scan merging with the
<code>SCAN_MERGING</code> table capability. Among the built-in file formats,
Parquet, ORC, text and Avro opt in on their V2 read path, which a format
reaches only when it is removed from
<code>spark.sql.sources.useV1SourceList</code>; there the strictly enforced
filters are the partition filters, so this configuration lets two scans over
the same partitions but with different data filters merge. A file table
withholds the capability when <code>spa
rk.sql.files.ignoreCorruptFiles</code> or
<code>spark.sql.files.ignoreMissingFiles</code> is true, because a read failure
in a column that only the other scan projects would then be swallowed along
with the rest of that file's rows, and Avro withholds it under
<code>positionalFieldMatching</code>, which resolves a column by its position
in the projection.
Review Comment:
**Finding 17.** This paragraph is the only place the new behaviour is
written down, and it sits in the cell of a configuration that is `false` by
default. That configuration only adds the differing-data-filter shape. The
projection-only merge and the two withholding rules hold whatever it is set to.
Meanwhile the prose a reader consults for when merging fires still states
the old rule.
`docs/sql-performance-tuning.md:343`:
> Two subplans are merged when their plans match node by node: ... and the
leaves must read the same input.
Line 331 opens the section the same way ("subplans that return a single row
and read the same input"). Both were accurate while this cell ended with "no
built-in source does". After this PR two leaves that read different column sets
merge, under the default configuration, for Parquet, ORC, text and Avro on the
V2 read path.
Suggest moving the format list and the withholding rules up into the prose,
after line 343:
```markdown
On the DataSource V2 read path a source can go further and declare the
`SCAN_MERGING` table capability, which lets two leaves that differ only in
their projected columns merge into a single scan reading the union of those
columns. Among the built-in file formats Parquet, ORC, text and Avro declare
it; a format reaches its V2 read path only when it is removed from
`spark.sql.sources.useV1SourceList`. A file table withholds the capability when
`spark.sql.files.ignoreCorruptFiles` is true, because a read failure in a
column that only the other subplan projects would then be swallowed along with
the rest of that file's rows, and when `spark.sql.files.ignoreMissingFiles` is
true, to match the strictness predicate the file reader uses. Avro withholds it
under `positionalFieldMatching`, which resolves a column by its position in the
projection.
```
and leaving this cell its first three sentences plus one line for what the
configuration itself adds:
```html
For a file source the strictly enforced filters are the partition filters,
so this configuration is what lets two scans over the same partitions but with
different data filters merge.
```
The `ignoreMissingFiles` wording above is finding 18's fix, folded in.
##########
docs/sql-performance-tuning.md:
##########
@@ -384,7 +384,7 @@ In TPC-DS benchmark runs, enabling symmetric filter
propagation made `q9` and `q
<td><code>spark.sql.optimizer.mergeSubplans.filterPropagation.dsv2SymmetricFilterPropagation.enabled</code></td>
<td>false</td>
<td>
- When true, two DataSource V2 scans that pushed the same strictly
enforced filters but carry different best-effort (post-scan) filters can be
merged even when
<code>spark.sql.optimizer.mergeSubplans.filterPropagation.symmetricFilterPropagation.enabled</code>
is false. In this case widening cannot change the set of rows the scan is
required to return, as the strict filters are re-pushed unchanged and the
enclosing <code>Filter</code> re-checks the rest above the scan. This applies
only to V2 sources that opt in to scan merging with the
<code>SCAN_MERGING</code> table capability; no built-in source does.
+ When true, two DataSource V2 scans that pushed the same strictly
enforced filters but carry different best-effort (post-scan) filters can be
merged even when
<code>spark.sql.optimizer.mergeSubplans.filterPropagation.symmetricFilterPropagation.enabled</code>
is false. In this case widening cannot change the set of rows the scan is
required to return, as the strict filters are re-pushed unchanged and the
enclosing <code>Filter</code> re-checks the rest above the scan. This applies
only to V2 sources that opt in to scan merging with the
<code>SCAN_MERGING</code> table capability. Among the built-in file formats,
Parquet, ORC, text and Avro opt in on their V2 read path, which a format
reaches only when it is removed from
<code>spark.sql.sources.useV1SourceList</code>; there the strictly enforced
filters are the partition filters, so this configuration lets two scans over
the same partitions but with different data filters merge. A file table
withholds the capability when <code>spa
rk.sql.files.ignoreCorruptFiles</code> or
<code>spark.sql.files.ignoreMissingFiles</code> is true, because a read failure
in a column that only the other scan projects would then be swallowed along
with the rest of that file's rows, and Avro withholds it under
<code>positionalFieldMatching</code>, which resolves a column by its position
in the projection.
Review Comment:
**Finding 18.** The "because" covers both flags as written, and only one of
them does that.
> A file table withholds the capability when
`spark.sql.files.ignoreCorruptFiles` or `spark.sql.files.ignoreMissingFiles` is
true, because a read failure in a column that only the other scan projects
would then be swallowed along with the rest of that file's rows
`ignoreMissingFiles` swallows a `FileNotFoundException`
(`sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/FilePartitionReader.scala:51`),
which is file-level: the same rows go whatever either side projects. Your own
scaladoc says it, `FileTable.scala:143-145`:
> `ignoreMissingFiles` drops the same rows whatever is projected, and is
included to match `FileScanRDD.hasStrictFileReads`, the same predicate on the
physical side.
So the reason belongs to `ignoreCorruptFiles`, and the second flag needs its
own half-clause - "and when `spark.sql.files.ignoreMissingFiles` is true, to
match the strictness predicate the file reader uses". Not reopening whether the
flag belongs in the predicate; that is settled.
--
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]
