You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2023/03/14 08:56:25 UTC
[GitHub] [spark] beliefer opened a new pull request, #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
beliefer opened a new pull request, #40410:
URL: https://github.com/apache/spark/pull/40410
### What changes were proposed in this pull request?
https://github.com/apache/spark/pull/40142 have an unrelated change and is actually a regression. The change let infer window group limit runs early.
Infer window group limit should run as late as possible, it is more safe.
### Why are the changes needed?
Infer window group limit should run as late as possible.
### Does this PR introduce _any_ user-facing change?
'No'.
New feature.
### How was this patch tested?
Exists test cases.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #40410:
URL: https://github.com/apache/spark/pull/40410#issuecomment-1469541110
ping @cloud-fan cc @zhengruifeng
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40410:
URL: https://github.com/apache/spark/pull/40410#discussion_r1135912257
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkOptimizer.scala:
##########
@@ -84,6 +84,11 @@ class SparkOptimizer(
PushPredicateThroughNonJoin,
PushProjectionThroughLimit,
RemoveNoopOperators) :+
+ Batch("Infer window group limit", Once,
+ InferWindowGroupLimit,
+ LimitPushDown,
+ LimitPushDownThroughWindow,
+ EliminateLimits) :+
Review Comment:
May I ask why this is different from #40142 ? At that time, we only remove one rule. So, I guess we will recover only `InferWindowGroupLimit` here.
```
- Batch("Insert window group limit", Once, InsertWindowGroupLimit) :+
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #40410:
URL: https://github.com/apache/spark/pull/40410#issuecomment-1470311818
Thank you, @beliefer and @cloud-fan .
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40410:
URL: https://github.com/apache/spark/pull/40410#discussion_r1135914874
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InferWindowGroupLimit.scala:
##########
@@ -84,11 +84,11 @@ object InferWindowGroupLimit extends Rule[LogicalPlan] with PredicateHelper {
}
// Pick a rank-like function with the smallest limit
selectedLimits.minBy(_._1) match {
- case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold =>
+ case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold &&
+ child.maxRows.forall(_ > limit) =>
Review Comment:
Is this inevitable when we move the rule?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #40410:
URL: https://github.com/apache/spark/pull/40410#discussion_r1136432237
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/SparkOptimizer.scala:
##########
@@ -84,6 +84,11 @@ class SparkOptimizer(
PushPredicateThroughNonJoin,
PushProjectionThroughLimit,
RemoveNoopOperators) :+
+ Batch("Infer window group limit", Once,
+ InferWindowGroupLimit,
+ LimitPushDown,
+ LimitPushDownThroughWindow,
+ EliminateLimits) :+
Review Comment:
https://github.com/apache/spark/pull/40142 just extracts the `Limit` from `Window`. But we still need other rules about `Limit` for further optimization. e.g. `LimitPushDownThroughWindow`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #40410:
URL: https://github.com/apache/spark/pull/40410#discussion_r1136923968
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InferWindowGroupLimit.scala:
##########
@@ -84,11 +84,11 @@ object InferWindowGroupLimit extends Rule[LogicalPlan] with PredicateHelper {
}
// Pick a rank-like function with the smallest limit
selectedLimits.minBy(_._1) match {
- case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold =>
+ case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold &&
+ child.maxRows.forall(_ > limit) =>
Review Comment:
In previous, the batch only contains `InferWindowGroupLimit`. After extracts one `Limit` between `Filter` and `Window`, so the plan not meet `InferWindowGroupLimit` again.
This PR added other rules into the same batch, so the `Limit` extracted by `InferWindowGroupLimit`, will be pushed down to `Window`. So the plan still meet `InferWindowGroupLimit`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40410:
URL: https://github.com/apache/spark/pull/40410#discussion_r1135912798
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -130,7 +130,6 @@ abstract class Optimizer(catalogManager: CatalogManager)
val operatorOptimizationBatch: Seq[Batch] = {
Batch("Operator Optimization before Inferring Filters", fixedPoint,
operatorOptimizationRuleSet: _*) ::
- Batch("Infer window group limit", Once, InferWindowGroupLimit) ::
Review Comment:
This is correctly.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #40410:
URL: https://github.com/apache/spark/pull/40410#discussion_r1136467297
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InferWindowGroupLimit.scala:
##########
@@ -84,11 +84,11 @@ object InferWindowGroupLimit extends Rule[LogicalPlan] with PredicateHelper {
}
// Pick a rank-like function with the smallest limit
selectedLimits.minBy(_._1) match {
- case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold =>
+ case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold &&
+ child.maxRows.forall(_ > limit) =>
Review Comment:
In previous, the idempotence not be exposed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] dongjoon-hyun commented on a diff in pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #40410:
URL: https://github.com/apache/spark/pull/40410#discussion_r1136438686
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InferWindowGroupLimit.scala:
##########
@@ -84,11 +84,11 @@ object InferWindowGroupLimit extends Rule[LogicalPlan] with PredicateHelper {
}
// Pick a rank-like function with the smallest limit
selectedLimits.minBy(_._1) match {
- case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold =>
+ case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold &&
+ child.maxRows.forall(_ > limit) =>
Review Comment:
So, before this reverting, the idempotence was ensured?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #40410:
URL: https://github.com/apache/spark/pull/40410#issuecomment-1469878236
thanks, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #40410:
URL: https://github.com/apache/spark/pull/40410#issuecomment-1469932642
@cloud-fan @dongjoon-hyun Thank you !
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #40410:
URL: https://github.com/apache/spark/pull/40410#discussion_r1136819813
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InferWindowGroupLimit.scala:
##########
@@ -84,11 +84,11 @@ object InferWindowGroupLimit extends Rule[LogicalPlan] with PredicateHelper {
}
// Pick a rank-like function with the smallest limit
selectedLimits.minBy(_._1) match {
- case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold =>
+ case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold &&
+ child.maxRows.forall(_ > limit) =>
Review Comment:
I'm also curious about why we suddenly detect idempotence issue. The batch is still Once.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #40410:
URL: https://github.com/apache/spark/pull/40410#discussion_r1136923968
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InferWindowGroupLimit.scala:
##########
@@ -84,11 +84,11 @@ object InferWindowGroupLimit extends Rule[LogicalPlan] with PredicateHelper {
}
// Pick a rank-like function with the smallest limit
selectedLimits.minBy(_._1) match {
- case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold =>
+ case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold &&
+ child.maxRows.forall(_ > limit) =>
Review Comment:
In previous, the batch only contains `InferWindowGroupLimit`. After extracts one Limit between Filter and Window, so the plan not meet `InferWindowGroupLimit` again.
This PR added other rules into the same batch, so the Limit extracted by `InferWindowGroupLimit`, will be pushed down to Window. So the plan still meet `InferWindowGroupLimit`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
URL: https://github.com/apache/spark/pull/40410
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] beliefer commented on a diff in pull request #40410: [SPARK-42783][SQL] Infer window group limit should run as late as possible
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #40410:
URL: https://github.com/apache/spark/pull/40410#discussion_r1136433968
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/InferWindowGroupLimit.scala:
##########
@@ -84,11 +84,11 @@ object InferWindowGroupLimit extends Rule[LogicalPlan] with PredicateHelper {
}
// Pick a rank-like function with the smallest limit
selectedLimits.minBy(_._1) match {
- case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold =>
+ case (limit, rankLikeFunction) if limit <= conf.windowGroupLimitThreshold &&
+ child.maxRows.forall(_ > limit) =>
Review Comment:
This is to ensure the rule's idempotence.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org