[SPARK-27514] Skip collapsing windows with empty window expressions #24411
Conversation
|
ok to test |
|
@dongjoon-hyun @cloud-fan - mind taking a look? |
|
@yifeih can we fix the PR title, it should be: [SPARK-27514][SQL] Skip collapsing windows with empty window expressions |
…ons (#538) ## Upstream SPARK-XXXXX ticket and PR link (if not applicable, explain) github: apache#24411 jira: https://issues.apache.org/jira/browse/SPARK-27514 ## What changes were proposed in this pull request? A previous change moved the removal of empty window expressions to the RemoveNoopOperations rule, which comes after the CollapseWindow rule. Therefore, by the time we get to CollapseWindow, we aren't guaranteed that empty windows have been removed. This change checks that the window expressions are not empty, and only collapses the windows if both windows are non-empty. A lengthier description and repro steps here: https://issues.apache.org/jira/browse/SPARK-27514 ## How was this patch tested? A unit test, plus I reran the breaking case mentioned in the Jira ticket.
| @@ -770,6 +770,7 @@ object CollapseWindow extends Rule[LogicalPlan] { | |||
| def apply(plan: LogicalPlan): LogicalPlan = plan transformUp { | |||
| case w1 @ Window(we1, ps1, os1, w2 @ Window(we2, ps2, os2, grandChild)) | |||
| if ps1 == ps2 && os1 == os2 && w1.references.intersect(w2.windowOutputSet).isEmpty && | |||
| we1.nonEmpty && we2.nonEmpty && | |||
There was a problem hiding this comment.
just for curiosity: do you know why we can't collapse windows with empty window expressions? Seems we can change WindowFunctionType.functionType(we1.head) == WindowFunctionType.functionType(we2.head) to we1.isEmpty || we2.isEmpty || WindowFunctionType.functionType(we1.head) == WindowFunctionType.functionType(we2.head)
There was a problem hiding this comment.
I think either change would be equivalent. If we don't collapse here, the rule that removes no-ops would prune out the empty window anyways, effectively resulting in the same collapsing behavior.
|
Test build #104731 has finished for PR 24411 at commit
|
|
thanks, merging to master! |
|
+1, Late LGTM. |
What changes were proposed in this pull request?
A previous change moved the removal of empty window expressions to the RemoveNoopOperations rule, which comes after the CollapseWindow rule. Therefore, by the time we get to CollapseWindow, we aren't guaranteed that empty windows have been removed. This change checks that the window expressions are not empty, and only collapses the windows if both windows are non-empty.
A lengthier description and repro steps here: https://issues.apache.org/jira/browse/SPARK-27514
How was this patch tested?
A unit test, plus I reran the breaking case mentioned in the Jira ticket.