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.

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

Aggregation pushdown for SQL Server #5196

Closed
wants to merge 9 commits into from
Closed

Aggregation pushdown for SQL Server #5196

wants to merge 9 commits into from

Conversation

mr-big-spency
Copy link

Resolve #4139

@cla-bot
Copy link

cla-bot bot commented Sep 16, 2020

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/prestosql/cla.

@findepi
Copy link
Member

findepi commented Sep 17, 2020

Can you please squash & rebase? There is a conflict which prevents CI from running on your PR.

@mosabua
Copy link
Member

mosabua commented Sep 17, 2020

Can you please add the relevant user facing documentation similar to e.g. https://prestosql.io/docs/current/connector/postgresql.html#pushdown

@findepi findepi changed the title Agg pushdown sqlserver Aggregation pushdown for SQL Server Sep 18, 2020
@findepi findepi added the enhancement New feature or request label Sep 18, 2020
@mr-big-spency
Copy link
Author

Can you please squash & rebase? There is a conflict which prevents CI from running on your PR.

yes. will do shortly.

@mr-big-spency
Copy link
Author

Can you please squash & rebase? There is a conflict which prevents CI from running on your PR.

yes. will do shortly.

Can you please add the relevant user facing documentation similar to e.g. https://prestosql.io/docs/current/connector/postgresql.html#pushdown

yes. will do. this is my first contribution... there must be a .md file to update for this release right?

@mosabua
Copy link
Member

mosabua commented Sep 18, 2020

Doc file to update it https://github.com/prestosql/presto/blob/master/presto-docs/src/main/sphinx/connector/sqlserver.rst

And with regards to release notes.. that is assembled later.

@mosabua mosabua closed this Sep 18, 2020
@mosabua mosabua reopened this Sep 18, 2020
@martint
Copy link
Member

martint commented Sep 18, 2020

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Sep 18, 2020
@cla-bot
Copy link

cla-bot bot commented Sep 18, 2020

The cla-bot has been summoned, and re-checked this pull request!

@dleach001
Merge branch 'master' into agg-pushdown-sqlserver
283e7c2 
# Conflicts:
#	presto-sqlserver/src/main/java/io/prestosql/plugin/sqlserver/SqlServerClient.java
@dleach001
implement pushdown logic as demonstrated in postgres example
5fe9c7e
@findepi findepi requested a review from aalbu September 19, 2020 14:53
aalbu
aalbu reviewed Sep 21, 2020
View reviewed changes
Copy link
Member

@aalbu aalbu left a comment

Choose a reason for hiding this comment

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

LGTM

presto-sqlserver/src/test/java/io/prestosql/plugin/sqlserver/TestSqlServerClient.java
Optional.empty());

// count() FILTER (WHERE ...)

Copy link
Member

Choose a reason for hiding this comment

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

Can you remove this empty line?

@findepi findepi mentioned this pull request Sep 21, 2020
Closed
@findepi
Copy link
Member

findepi commented Sep 21, 2020

merged as 5bdd1c1, thanks!

@findepi findepi closed this Sep 21, 2020
@findepi findepi added this to the 342 milestone Sep 21, 2020
@findepi findepi mentioned this pull request Sep 21, 2020
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aalbu aalbu aalbu left review comments

@findepi findepi findepi approved these changes

Assignees
No one assigned
Labels
cla-signed enhancement New feature or request
Milestone
342
Development

Successfully merging this pull request may close these issues.

Aggregation pushdown in SQL Server connector
6 participants
@mr-big-spency @findepi @mosabua @martint @aalbu @dleach001