-
Notifications
You must be signed in to change notification settings - Fork 28.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-33302][SQL] Push down filters through Expand #30278
Conversation
Test build #130714 has finished for PR 30278 at commit
|
Kubernetes integration test starting |
retest this please |
Kubernetes integration test status failure |
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #130723 has finished for PR 30278 at commit
|
gentle ping @maropu @cloud-fan |
@@ -1269,6 +1269,7 @@ object PushPredicateThroughNonJoin extends Rule[LogicalPlan] with PredicateHelpe | |||
case _: Sort => true | |||
case _: BatchEvalPython => true | |||
case _: ArrowEvalPython => true | |||
case _: Expand => true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change affects the PushDownLeftSemiAntiJoin
rule, too. So, could you add tests for the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change affects the
PushDownLeftSemiAntiJoin
rule, too. So, could you add tests for the case?
Double check the case, seems current master fix this case by some pr, but 3.0 is still as jira desc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems current master fix this case
What do you mean by "fix this case"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems current master fix this case
What do you mean by "fix this case"?
I have found the pr #29673
Before this pr, SQL
SELECT
years,
appversion,
SUM(uusers) AS users
FROM (SELECT
Date_trunc('year', dt) AS years,
CASE
WHEN h.pid = 3 THEN 'iOS'
WHEN h.pid = 4 THEN 'Android'
ELSE 'Other'
END AS viewport,
h.vs AS appversion,
Count(DISTINCT u.uid) AS uusers
,Count(DISTINCT u.suid) AS srcusers
FROM t1 u
join t2 h
ON h.uid = u.uid
GROUP BY 1,
2,
3) AS a
WHERE viewport = 'iOS'
GROUP BY 1,
2
Optimized plan is
== Optimized Logical Plan ==
Aggregate [years#0, appversion#2], [years#0, appversion#2, sum(uusers#3L) AS users#5L]
+- Aggregate [date_trunc('year', CAST(u.`dt` AS TIMESTAMP))#24, CASE WHEN (h.`pid` = 3) THEN 'iOS' WHEN (h.`pid` = 4) THEN 'Android' ELSE 'Other' END#25, vs#17], [date_trunc('year', CAST(u.`dt` AS TIMESTAMP))#24 AS years#0, vs#17 AS appversion#2, count(if ((gid#23 = 1)) u.`uid`#26 else null) AS uusers#3L]
+- Aggregate [date_trunc('year', CAST(u.`dt` AS TIMESTAMP))#24, CASE WHEN (h.`pid` = 3) THEN 'iOS' WHEN (h.`pid` = 4) THEN 'Android' ELSE 'Other' END#25, vs#17, u.`uid`#26, u.`suid`#27, gid#23], [date_trunc('year', CAST(u.`dt` AS TIMESTAMP))#24, CASE WHEN (h.`pid` = 3) THEN 'iOS' WHEN (h.`pid` = 4) THEN 'Android' ELSE 'Other' END#25, vs#17, u.`uid`#26, gid#23]
+- Filter (CASE WHEN (h.`pid` = 3) THEN 'iOS' WHEN (h.`pid` = 4) THEN 'Android' ELSE 'Other' END#25 = iOS)
+- Expand [ArrayBuffer(date_trunc(year, cast(dt#14 as timestamp), Some(Asia/Shanghai)), CASE WHEN (pid#16 = 3) THEN iOS WHEN (pid#16 = 4) THEN Android ELSE Other END, vs#17, uid#12, null, 1), ArrayBuffer(date_trunc(year, cast(dt#14 as timestamp), Some(Asia/Shanghai)), CASE WHEN (pid#16 = 3) THEN iOS WHEN (pid#16 = 4) THEN Android ELSE Other END, vs#17, null, suid#15, 2)], [date_trunc('year', CAST(u.`dt` AS TIMESTAMP))#24, CASE WHEN (h.`pid` = 3) THEN 'iOS' WHEN (h.`pid` = 4) THEN 'Android' ELSE 'Other' END#25, vs#17, u.`uid`#26, u.`suid`#27, gid#23]
+- Project [uid#12, dt#14, suid#15, pid#16, vs#17]
+- Join Inner, (uid#18 = uid#12)
:- Project [uid#12, dt#14, suid#15]
: +- Filter isnotnull(uid#12)
: +- Relation[pid#11,uid#12,sid#13,dt#14,suid#15] parquet
+- Project [pid#16, vs#17, uid#18]
+- Filter isnotnull(uid#18)
+- Relation[pid#16,vs#17,uid#18,csid#19] parquet
After that pr, Optimized plan is
== Optimized Logical Plan ==
Aggregate [years#0, appversion#2], [years#0, appversion#2, sum(uusers#3L) AS users#5L]
+- Aggregate [date_trunc(year, cast(dt#14 as timestamp), Some(Asia/Shanghai)), CASE WHEN (pid#16 = 3) THEN iOS WHEN (pid#16 = 4) THEN Android ELSE Other END, vs#17], [date_trunc(year, cast(dt#14 as timestamp), Some(Asia/Shanghai)) AS years#0, vs#17 AS appversion#2, count(distinct uid#12) AS uusers#3L]
+- Project [uid#12, dt#14, pid#16, vs#17]
+- Join Inner, (uid#18 = uid#12)
:- Project [uid#12, dt#14]
: +- Filter isnotnull(uid#12)
: +- Relation[pid#11,uid#12,sid#13,dt#14,suid#15] parquet
+- Project [pid#16, vs#17, uid#18]
+- Filter ((CASE WHEN (pid#16 = 3) THEN iOS WHEN (pid#16 = 4) THEN Android ELSE Other END = iOS) AND isnotnull(uid#18))
+- Relation[pid#16,vs#17,uid#18,csid#19] parquet
Filter((CASE WHEN (pid#16 = 3) THEN iOS WHEN (pid#16 = 4) THEN Android ELSE Other END = iOS))
is pushed down and won't generate Expand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does it related to left semi join?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change affects the PushDownLeftSemiAntiJoin rule, too. So, could you add tests for the case?
So can we add such a test?
With test case in LeftSemiPushdownSuite
test("Unary: LeftSemi join push down through expand") {
val expand = Expand(Seq(Seq('a, 'b, "null"), Seq('a, "null", 'c)),
Seq('a, 'b, 'c), testRelation)
val originalQuery = expand
.join(testRelation1, joinType = LeftSemi, condition = Some('b === 'd && 'b === 1))
val optimized = Optimize.execute(originalQuery.analyze)
val correctAnswer = Expand(Seq(Seq('a, 'b, "null"), Seq('a, "null", 'c)),
Seq('a, 'b, 'c), Filter(EqualTo('b, 1), testRelation))
.join(testRelation1, joinType = LeftSemi, condition = Some('b === 'd))
.analyze
comparePlans(optimized, correctAnswer)
}
originalQuery is
'Join LeftSemi, (('b = 'd) AND ('b = 1))
:- 'Expand [List('a, 'b, null), List('a, null, 'c)], ['a, 'b, 'c]
: +- LocalRelation <empty>, [a#0, b#1, c#2]
+- LocalRelation <empty>, [d#3]
Test result is
== FAIL: Plans do not match ===
!'Expand [List(a#0, b#0, null), List(a#0, null, c#0)], [a#0, b#0, c#0] 'Join LeftSemi, (b#0 = d#0)
!+- 'Join LeftSemi, ((b#0 = 1) AND (b#0 = d#0)) :- Expand [List(a#0, b#0, null), List(a#0, null, c#0)], [a#0, b#0, c#0]
! :- LocalRelation <empty>, [a#0, b#0, c#0] : +- Filter (b#0 = 1)
! +- LocalRelation <empty>, [d#0] : +- LocalRelation <empty>, [a#0, b#0, c#0]
! +- LocalRelation <empty>, [d#0]
Expand will be promoted below Join, so should we ignore this case or add a parameter in canPushThrough
like below
def canPushThrough(p: UnaryNode, isFilterPushDown: Boolean = false): Boolean = p match {
// Note that some operators (e.g. project, aggregate, union) are being handled separately
// (earlier in this rule).
case _: AppendColumns => true
case _: Distinct => true
case _: Generate => true
case _: Pivot => true
case _: RepartitionByExpression => true
case _: Repartition => true
case _: ScriptTransformation => true
case _: Sort => true
case _: BatchEvalPython => true
case _: ArrowEvalPython => true
case _: Expand => isFilterPushDown
case _ => false
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry I didn't get it. What's the issue here? We can't pushdown left-semi join through expand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The optimized (left-side) plan above looks correct to me...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry I didn't get it. What's the issue here? We can't pushdown left-semi join through expand?
oh..my mistake, I misunderstood some code about PushDownLeftSemiAntiJoin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The optimized (left-side) plan above looks correct to me...
My fault, I misunderstand some code about PushDownLeftSemiAntiJoin
, test case added ==
test("push down predicate through expand") { | ||
val input = LocalRelation('a.int, 'b.string, 'c.double) | ||
val query = | ||
Aggregate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this test need an Aggregate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this test need an Aggregate?
Not necessary, remove it.
Kubernetes integration test starting |
Kubernetes integration test status success |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #130787 has finished for PR 30278 at commit
|
Test build #130797 has finished for PR 30278 at commit
|
@@ -1208,6 +1208,30 @@ class FilterPushdownSuite extends PlanTest { | |||
checkAnalysis = false) | |||
} | |||
|
|||
|
|||
test("push down predicate through expand") { | |||
val input = LocalRelation('a.int, 'b.string, 'c.double) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could you use testRelation
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could you use
testRelation
instead?
Done
@@ -1208,6 +1208,30 @@ class FilterPushdownSuite extends PlanTest { | |||
checkAnalysis = false) | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unnecessary blank.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: unnecessary blank.
Done
typo? |
LGTM except for the minor comments. |
Yea, updated |
Kubernetes integration test starting |
Kubernetes integration test status failure |
Test build #130808 has finished for PR 30278 at commit
|
retest this please |
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #130832 has finished for PR 30278 at commit
|
retest this please |
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #130850 has finished for PR 30278 at commit
|
thanks, merging to master! |
What changes were proposed in this pull request?
Push down filter through expand. For case below:
Plan. before this pr:
Plan. after. this pr. :
Why are the changes needed?
Improve performance, filter more data.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added UT