You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zml1206 (via GitHub)" <gi...@apache.org> on 2024/02/01 03:45:31 UTC
[PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
zml1206 opened a new pull request, #44975:
URL: https://github.com/apache/spark/pull/44975
### What changes were proposed in this pull request?
Push down throwable predicate through aggregates.
### Why are the changes needed?
Push down throwable filter through aggregate does not affect exception thrown.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
UT
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1474497974
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1733,8 +1733,8 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
// attributes produced by the aggregate operator's child operator.
val (pushDown, stayUp) = splitConjunctivePredicates(condition).partition { cond =>
val replaced = replaceAlias(cond, aliasMap)
- cond.deterministic && !cond.throwable &&
- cond.references.nonEmpty && replaced.references.subsetOf(aggregate.child.outputSet)
+ cond.deterministic && cond.references.nonEmpty &&
Review Comment:
PR decription updated.
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #44975:
URL: https://github.com/apache/spark/pull/44975#issuecomment-1923022300
cc @kelvinjian-db
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1474621382
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1733,8 +1733,8 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
// attributes produced by the aggregate operator's child operator.
val (pushDown, stayUp) = splitConjunctivePredicates(condition).partition { cond =>
val replaced = replaceAlias(cond, aliasMap)
- cond.deterministic && !cond.throwable &&
- cond.references.nonEmpty && replaced.references.subsetOf(aggregate.child.outputSet)
+ cond.deterministic && cond.references.nonEmpty &&
Review Comment:
Can we add code comments above `case filter @ Filter(condition, aggregate: Aggregate)`?
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1475442089
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/FilterPushdownSuite.scala:
##########
@@ -1459,12 +1470,15 @@ class FilterPushdownSuite extends PlanTest {
test("SPARK-46707: push down predicate with sequence (without step) through aggregates") {
val x = testRelation.subquery("x")
- // do not push down when sequence has step param
Review Comment:
maybe just say `Always pushdown sequence as it's deterministic` and remove https://github.com/apache/spark/pull/44975/files#diff-b469906c633fbbcd0d261933b55c09455a94e26ca967b922d1c569c2fcb16415R1483
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on PR #44975:
URL: https://github.com/apache/spark/pull/44975#issuecomment-1920934801
cc @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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1475792513
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1733,8 +1737,8 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
// attributes produced by the aggregate operator's child operator.
val (pushDown, stayUp) = splitConjunctivePredicates(condition).partition { cond =>
val replaced = replaceAlias(cond, aliasMap)
- cond.deterministic && !cond.throwable &&
Review Comment:
Do we need add a test case about the throwable condition?
cc @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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on PR #44975:
URL: https://github.com/apache/spark/pull/44975#issuecomment-2109527145
Thank you all for review. @cloud-fan @kelvinjian-db @beliefer
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #44975:
URL: https://github.com/apache/spark/pull/44975#issuecomment-2109516986
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1475579133
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1724,6 +1724,10 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
val aliasMap = getAliasMap(project)
project.copy(child = Filter(replaceAlias(condition, aliasMap), grandChild))
+ // We can push down deterministic predicate through Aggregate, including throwable predicate.
+ // If we can push down a filter through Aggregate, it means the filter only references the
+ // grouping keys. The Aggregate operator can't reduce grouping keys so the filter won't see any
Review Comment:
e.g. `Filter(a > 1, Aggregate(1 as a, max(col), ...))`, we can push down the filter, right?
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1474273894
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1733,8 +1733,8 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
// attributes produced by the aggregate operator's child operator.
val (pushDown, stayUp) = splitConjunctivePredicates(condition).partition { cond =>
val replaced = replaceAlias(cond, aliasMap)
- cond.deterministic && !cond.throwable &&
- cond.references.nonEmpty && replaced.references.subsetOf(aggregate.child.outputSet)
+ cond.deterministic && cond.references.nonEmpty &&
Review Comment:
Is it possible that `cond` is simpler than `replaced`?
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1474681372
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1733,8 +1733,8 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
// attributes produced by the aggregate operator's child operator.
val (pushDown, stayUp) = splitConjunctivePredicates(condition).partition { cond =>
val replaced = replaceAlias(cond, aliasMap)
- cond.deterministic && !cond.throwable &&
- cond.references.nonEmpty && replaced.references.subsetOf(aggregate.child.outputSet)
+ cond.deterministic && cond.references.nonEmpty &&
Review Comment:
Added.
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1475456596
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1724,6 +1724,10 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
val aliasMap = getAliasMap(project)
project.copy(child = Filter(replaceAlias(condition, aliasMap), grandChild))
+ // We can push down deterministic predicate through Aggregate, including throwable predicate.
+ // If we can push down a filter through Aggregate, it means the filter only references the
+ // grouping keys. The Aggregate operator can't reduce grouping keys so the filter won't see any
Review Comment:
why need "or constants" when `replaced.references.subsetOf(aggregate.child.outputSet)`? `Expression.references` does not contain constants.
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #44975:
URL: https://github.com/apache/spark/pull/44975#issuecomment-2106428607
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1474474986
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1733,8 +1733,8 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
// attributes produced by the aggregate operator's child operator.
val (pushDown, stayUp) = splitConjunctivePredicates(condition).partition { cond =>
val replaced = replaceAlias(cond, aliasMap)
- cond.deterministic && !cond.throwable &&
- cond.references.nonEmpty && replaced.references.subsetOf(aggregate.child.outputSet)
+ cond.deterministic && cond.references.nonEmpty &&
Review Comment:
I think we need to document these hidden assumptions, otherwise code changes here are hard to review.
If we can push down a filter through Aggregate, it means the filter only references the grouping keys. The Aggregate operator can't reduce grouping keys so the filter won't see any new data after pushing down.
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1474266458
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1733,8 +1733,8 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
// attributes produced by the aggregate operator's child operator.
val (pushDown, stayUp) = splitConjunctivePredicates(condition).partition { cond =>
val replaced = replaceAlias(cond, aliasMap)
- cond.deterministic && !cond.throwable &&
- cond.references.nonEmpty && replaced.references.subsetOf(aggregate.child.outputSet)
+ cond.deterministic && cond.references.nonEmpty &&
Review Comment:
I think `cond.deterministic` and `replaced.deterministic` are equivalent when `aggregate.aggregateExpressions.forall(_.deterministic)`.
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1475456596
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1724,6 +1724,10 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
val aliasMap = getAliasMap(project)
project.copy(child = Filter(replaceAlias(condition, aliasMap), grandChild))
+ // We can push down deterministic predicate through Aggregate, including throwable predicate.
+ // If we can push down a filter through Aggregate, it means the filter only references the
+ // grouping keys. The Aggregate operator can't reduce grouping keys so the filter won't see any
Review Comment:
why need "or constants" when `replaced.references.subsetOf(aggregate.child.outputSet)`?
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1475441360
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1724,6 +1724,10 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
val aliasMap = getAliasMap(project)
project.copy(child = Filter(replaceAlias(condition, aliasMap), grandChild))
+ // We can push down deterministic predicate through Aggregate, including throwable predicate.
+ // If we can push down a filter through Aggregate, it means the filter only references the
+ // grouping keys. The Aggregate operator can't reduce grouping keys so the filter won't see any
Review Comment:
```suggestion
// grouping keys or constants. The Aggregate operator can't reduce distinct values of grouping keys so the filter won't see any
```
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1475595700
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1724,6 +1724,10 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
val aliasMap = getAliasMap(project)
project.copy(child = Filter(replaceAlias(condition, aliasMap), grandChild))
+ // We can push down deterministic predicate through Aggregate, including throwable predicate.
+ // If we can push down a filter through Aggregate, it means the filter only references the
+ // grouping keys. The Aggregate operator can't reduce grouping keys so the filter won't see any
Review Comment:
yea, updated.
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1474260366
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1733,8 +1733,8 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
// attributes produced by the aggregate operator's child operator.
val (pushDown, stayUp) = splitConjunctivePredicates(condition).partition { cond =>
val replaced = replaceAlias(cond, aliasMap)
- cond.deterministic && !cond.throwable &&
- cond.references.nonEmpty && replaced.references.subsetOf(aggregate.child.outputSet)
+ cond.deterministic && cond.references.nonEmpty &&
Review Comment:
I thought we wanted to change `cond` to `replaced`, but this reverts https://github.com/apache/spark/commit/62956c92cfc74d7523328d168b6d837938cde763 , why?
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #44975: [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates
URL: https://github.com/apache/spark/pull/44975
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #44975: [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates
URL: https://github.com/apache/spark/pull/44975
--
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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on PR #44975:
URL: https://github.com/apache/spark/pull/44975#issuecomment-2106524140
Is there still any problem with this PR, Can it be merged? Can you help take a look @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
Re: [PR] [SPARK-46707][SQL][FOLLOWUP] Push down throwable predicate through aggregates [spark]
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #44975:
URL: https://github.com/apache/spark/pull/44975#discussion_r1474273894
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -1733,8 +1733,8 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe
// attributes produced by the aggregate operator's child operator.
val (pushDown, stayUp) = splitConjunctivePredicates(condition).partition { cond =>
val replaced = replaceAlias(cond, aliasMap)
- cond.deterministic && !cond.throwable &&
- cond.references.nonEmpty && replaced.references.subsetOf(aggregate.child.outputSet)
+ cond.deterministic && cond.references.nonEmpty &&
Review Comment:
Is it possible that `cond` is simpler than `replaced` then has fewer iterations ?
--
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