Skip to content

[SPARK-34952][SQL][FOLLOW-UP] DSv2 aggregate push down follow-up #33526

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.

Sign up for GitHub

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

Jump to bottom
Closed
huaxingao wants to merge 5 commits into from
Closed

[SPARK-34952][SQL][FOLLOW-UP] DSv2 aggregate push down follow-up #33526

huaxingao wants to merge 5 commits into from

Conversation

@huaxingao
Copy link
Contributor

@huaxingao huaxingao commented Jul 26, 2021 •
edited
Loading

What changes were proposed in this pull request?

update java doc, JDBC data source doc, address follow up comments

Why are the changes needed?

update doc and address follow up comments

Does this PR introduce any user-facing change?

Yes, add the new JDBC option pushDownAggregate in JDBC data source doc.

How was this patch tested?

manually checked

@huaxingao
Copy link
Contributor Author

huaxingao commented Jul 26, 2021

cc @cloud-fan @viirya

viirya
viirya previously approved these changes Jul 26, 2021
View reviewed changes
@viirya viirya mentioned this pull request Jul 26, 2021
Closed
@SparkQA
Copy link

SparkQA commented Jul 26, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46179/

@SparkQA
Copy link

SparkQA commented Jul 26, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46179/

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Test build #141663 has finished for PR 33526 at commit 4a37da5.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@huaxingao huaxingao changed the title [SPARK-34952][SQL][FOLLOW-UP] DSv2 aggregate push down: update doc [SPARK-34952][SQL][FOLLOW-UP] DSv2 aggregate push down follow-up Jul 27, 2021
@huaxingao
Copy link
Contributor Author

huaxingao commented Jul 27, 2021

cc @sunchao

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46190/

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46190/

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46191/

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46191/

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Test build #141674 has finished for PR 33526 at commit 21ef1a1.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Test build #141675 has finished for PR 33526 at commit 6e73fb3.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya viirya self-requested a review July 27, 2021 06:53
@viirya viirya dismissed their stale review July 27, 2021 06:54

there are new changes

cloud-fan
cloud-fan reviewed Jul 27, 2021
View reviewed changes
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/Aggregation.java Outdated
Copy link
Contributor

@cloud-fan cloud-fan Jul 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

shall we change the code style of groupByColumns as well?

Copy link
Contributor Author

@huaxingao huaxingao Jul 27, 2021

Choose a reason for hiding this comment

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

Sorry, the method was automatically collapsed into one single line in my IDE, I only saw one line there. I tried to be careful, but still missed this one.

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46225/

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46225/

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Test build #141712 has finished for PR 33526 at commit d204505.

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

sunchao
sunchao approved these changes Jul 27, 2021
View reviewed changes
Copy link
Member

@sunchao sunchao left a comment

Choose a reason for hiding this comment

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

LGTM (non-binding) with one nit. Thanks @huaxingao !

sql/catalyst/src/main/java/org/apache/spark/sql/connector/read/SupportsPushDownAggregates.java Outdated
* be: grouping columns, aggregate columns (in the same order as the aggregate functions in
* the given Aggregation).
*
* Returns true if the aggregation can be pushed down to datasource.
Copy link
Member

@sunchao sunchao Jul 27, 2021

Choose a reason for hiding this comment

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

nit: maybe "@return true if the aggregation can be pushed down to datasource. False otherwise."

Copy link
Contributor Author

@huaxingao huaxingao Jul 27, 2021

Choose a reason for hiding this comment

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

Done. Thanks!

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46248/

@SparkQA
Copy link

SparkQA commented Jul 27, 2021

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46248/

@SparkQA
Copy link

SparkQA commented Jul 28, 2021

Test build #141735 has finished for PR 33526 at commit 90356c8.

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

@cloud-fan
Copy link
Contributor

cloud-fan commented Jul 28, 2021

thanks, merging to master/3.2!

@cloud-fan cloud-fan closed this in c8dd97d Jul 28, 2021
cloud-fan pushed a commit that referenced this pull request Jul 28, 2021
@huaxingao @cloud-fan
[SPARK-34952][SQL][FOLLOW-UP] DSv2 aggregate push down follow-up
33ef52e 
### What changes were proposed in this pull request?
update java doc, JDBC data source doc, address follow up comments

### Why are the changes needed?
update doc and address follow up comments

### Does this PR introduce _any_ user-facing change?
Yes, add the new JDBC option `pushDownAggregate` in JDBC data source doc.

### How was this patch tested?
manually checked

Closes #33526 from huaxingao/aggPD_followup.

Authored-by: Huaxin Gao <[email protected]>
Signed-off-by: Wenchen Fan <[email protected]>
(cherry picked from commit c8dd97d)
Signed-off-by: Wenchen Fan <[email protected]>
@huaxingao huaxingao deleted the aggPD_followup branch July 28, 2021 05:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sunchao sunchao sunchao approved these changes

@cloud-fan cloud-fan cloud-fan approved these changes

@viirya viirya Awaiting requested review from viirya

Assignees

No one assigned

Labels

DOCS SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@huaxingao @SparkQA @cloud-fan @viirya @sunchao