peter-toth commented on code in PR #57602:
URL: https://github.com/apache/spark/pull/57602#discussion_r3685069865
##########
docs/sql-performance-tuning.md:
##########
@@ -136,6 +136,18 @@ Configuration of in-memory caching can be done via
`spark.conf.set` or by runnin
</td>
<td>2.1.1</td>
</tr>
+ <tr>
+ <td><code>spark.sql.execution.bypassPartialWindowGroupLimit</code></td>
Review Comment:
**Finding 3.** This row is the last entry of the "Tuning Partitions" table
(`spark.sql.files.*`, `spark.sql.shuffle.partitions`,
`spark.sql.sources.parallelPartitionDiscovery.*`), which is about how input and
shuffle partitions get sized. A window top-k planning flag isn't that, and
someone looking for window tuning won't find it there.
The page is organised as topical sections that each own a table (`## Tuning
Partitions`, `## Optimizing the Join Strategy`, `## Storage Partition Join`),
so the cleanest fix is a short section of its own -- e.g. a `## Tuning Window
Functions` after `## Tuning Partitions`, with a one-line intro and this row.
That would also be the natural home for
`spark.sql.execution.bypassPartialAggregation`, which is undocumented on this
page today, though that's not this PR's job.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -4527,6 +4527,17 @@ object SQLConf {
"The threshold of window group limit must be -1, 0 or positive
integer.")
.createWithDefault(1000)
+ val BYPASS_PARTIAL_WINDOW_GROUP_LIMIT =
+ buildConf("spark.sql.execution.bypassPartialWindowGroupLimit")
+ .doc("When true, skips the pre-shuffle partial WindowGroupLimit and runs
only a single " +
+ "WindowGroupLimit after the shuffle. Bypassing the partial window
group limit can " +
+ "improve performance when the pre-shuffle reduction ratio is low. When
false (default), " +
+ "a partial WindowGroupLimit runs before the shuffle and a final one
runs after it.")
+ .version("4.3.0")
+ .withBindingPolicy(ConfigBindingPolicy.SESSION)
Review Comment:
**Finding 4.** Step 1 of the `ConfigBindingPolicy` decision procedure is
"Can the config change the result of resolving the body of a
view/UDF/procedure, i.e. the resolved plan? If not, use `NOT_APPLICABLE`", with
the explicit note that "even physical planning or runtime configs may be read
while resolving a view, but they do not change what the body resolves to". This
one is read in `SparkStrategies`, after the body is resolved and optimized, so
`NOT_APPLICABLE` looks like the right declaration -- as it is for
`USE_HASH_AGG` (`:4091`, also a physical-planning operator choice) and for
`WINDOW_SEGMENT_TREE_ENABLED` just below (`:4543`).
Behaviour is the same either way (`NOT_APPLICABLE` also reads from the
active session), so this is purely about the declaration being right. I assume
it was copied from `BYPASS_PARTIAL_AGGREGATION` (`:4103`), which by the same
reading looks mis-declared too -- separate cleanup, not this PR.
```suggestion
.withBindingPolicy(ConfigBindingPolicy.NOT_APPLICABLE)
```
--
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]
