cloud-fan commented on code in PR #57602:
URL: https://github.com/apache/spark/pull/57602#discussion_r3688815804
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -4527,6 +4527,20 @@ 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")
Review Comment:
Please make this a query-level hint instead of a public session config. The
payoff depends on each query's `limit` and per-key distribution, so one session
value can improve one window query while regressing another; hints already
provide a logical-plan-local control surface (`UnresolvedHint` carries its
child). Carry a dedicated hint to the generated `WindowGroupLimit`, define its
behavior when a query has multiple windows, and cover that scope in tests.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkStrategies.scala:
##########
@@ -802,10 +802,25 @@ abstract class SparkStrategies extends
QueryPlanner[SparkPlan] {
object WindowGroupLimit extends Strategy {
def apply(plan: LogicalPlan): Seq[SparkPlan] = plan match {
case logical.WindowGroupLimit(partitionSpec, orderSpec,
rankLikeFunction, limit, child) =>
- val partialWindowGroupLimit =
execution.window.WindowGroupLimitExec(partitionSpec,
- orderSpec, rankLikeFunction, limit, execution.window.Partial,
planLater(child))
+ // When the partial window group limit is bypassed, skip the
pre-shuffle partial
+ // WindowGroupLimit and run only a single WindowGroupLimit after the
shuffle. This can
+ // improve performance when the pre-shuffle reduction ratio is low.
+ //
+ // The bypass is only gated on a non-empty partitionSpec. With an
empty partitionSpec the
+ // final WindowGroupLimit requires AllTuples, so the shuffle funnels
the whole input into a
+ // single reducer; the partial pass we would drop bounds each input
partition to `limit`
+ // rank groups before that shuffle, so it always helps and should
never be skipped.
Review Comment:
Please narrow this claim to multi-partition inputs. An
already-single-partition child satisfies `AllTuples` without a shuffle
(`EnsureRequirements.scala:71`), so Partial cannot reduce shuffle input there
and only adds another `WindowGroupLimit` pass. The guard is still a sensible
default, but `always helps` and `strictly worse` overstate its rationale; the
config docs make the same unconditional assumption.
--
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]
