Skip to content

[SPARK-27986][SQL][followup] window aggregate function with filter predicate is not supported - #27476

Closed
cloud-fan wants to merge 1 commit into
apache:masterfrom
cloud-fan:filter
Closed

[SPARK-27986][SQL][followup] window aggregate function with filter predicate is not supported#27476
cloud-fan wants to merge 1 commit into
apache:masterfrom
cloud-fan:filter

Conversation

@cloud-fan

@cloud-fancloud-fan commented Feb 6, 2020

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This is a followup of #26656.

We don't support window aggregate function with filter predicate yet and we should fail explicitly.

Observable metrics has the same issue. This PR fixes it as well.

Why are the changes needed?

If we simply ignore filter predicate when we don't support it, the result is wrong.

Does this PR introduce any user-facing change?

yea, fix the query result.

How was this patch tested?

new tests

@cloud-fan

Copy link
Copy Markdown
ContributorAuthor

cc @beliefer@maropu

@maropumaropu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, pending Jenkins.

@beliefer

Copy link
Copy Markdown
Contributor

Thank you. LGTM.

e.failAnalysis(
"distinct aggregates are not allowed in observed metrics, but found: " + s.sql)
case a: AggregateExpression if a.filter.isDefined =>
e.failAnalysis("aggregates with filter predicate are not allowed in " +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I lost the information. Thank you.

@SparkQA

Copy link
Copy Markdown

Test build #117990 has finished for PR 27476 at commit e9ec6ef.

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

@SparkQA

Copy link
Copy Markdown

Test build #117991 has finished for PR 27476 at commit e996e03.

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

@viiryaviirya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

good catch!

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1, LGTM. Thank you all.
Merged to master/3.0.

dongjoon-hyun pushed a commit that referenced this pull request Feb 6, 2020
…edicate is not supported
### What changes were proposed in this pull request?
This is a followup of #26656.
We don't support window aggregate function with filter predicate yet and we should fail explicitly.
Observable metrics has the same issue. This PR fixes it as well.
### Why are the changes needed?
If we simply ignore filter predicate when we don't support it, the result is wrong.
### Does this PR introduce any user-facing change?
yea, fix the query result.
### How was this patch tested?
new tests
Closes#27476 from cloud-fan/filter.
Authored-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
(cherry picked from commit 5a4c70b)
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
sjincho pushed a commit to sjincho/spark that referenced this pull request Apr 15, 2020
…edicate is not supported
### What changes were proposed in this pull request?
This is a followup of apache#26656.
We don't support window aggregate function with filter predicate yet and we should fail explicitly.
Observable metrics has the same issue. This PR fixes it as well.
### Why are the changes needed?
If we simply ignore filter predicate when we don't support it, the result is wrong.
### Does this PR introduce any user-facing change?
yea, fix the query result.
### How was this patch tested?
new tests
Closesapache#27476 from cloud-fan/filter.
Authored-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Sign up for freeto 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.

6 participants

@cloud-fan@beliefer@SparkQA@viirya@maropu@dongjoon-hyun