Skip to content
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-27514] Skip collapsing windows with empty window expressions #24411

Closed
wants to merge 1 commit into from

Conversation

yifeih
Copy link
Contributor

@yifeih yifeih commented Apr 18, 2019

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.

@mccheah
Copy link
Contributor

mccheah commented Apr 18, 2019

ok to test

@mccheah
Copy link
Contributor

mccheah commented Apr 18, 2019

@dongjoon-hyun @cloud-fan - mind taking a look?

@mccheah
Copy link
Contributor

mccheah commented Apr 18, 2019

@yifeih can we fix the PR title, it should be:

[SPARK-27514][SQL] Skip collapsing windows with empty window expressions

bulldozer-bot bot pushed a commit to palantir/spark that referenced this pull request Apr 18, 2019
…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 &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah i see, makes sense!

@SparkQA
Copy link

SparkQA commented Apr 19, 2019

Test build #104731 has finished for PR 24411 at commit 9fc933e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 163a6e2 Apr 19, 2019
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Apr 19, 2019

+1, Late LGTM.
Thank you for pinging me, @mccheah . Thank you, @yifeih and @cloud-fan .
cc @dbtsai .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants