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

Support aggregation pushdown in ClickHouse connector #7219 #7434

Merged
merged 1 commit into from
May 12, 2021
Merged

Support aggregation pushdown in ClickHouse connector #7219 #7434

merged 1 commit into from
May 12, 2021

Conversation

wgzhao
Copy link
Member

@wgzhao wgzhao commented Mar 26, 2021

Support aggregation pushdown in ClickHouse connector #7219

@cla-bot cla-bot bot added the cla-signed label Mar 26, 2021
@findepi findepi requested a review from hashhar March 26, 2021 17:50
hashhar
hashhar reviewed Apr 3, 2021
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Some tests need updating. LGTM once the tests are updated.

...n/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java Show resolved Hide resolved
@wgzhao wgzhao force-pushed the ck_support_aggregation_pushdown branch from 4274a54 to 46432d3 Compare April 6, 2021 14:26
@jerryleooo
Copy link
Member

@wgzhao looks like the CI failure is temporary, can just trigger another round of checks?

@wgzhao wgzhao force-pushed the ck_support_aggregation_pushdown branch 3 times, most recently from f387e9e to f3da27f Compare May 11, 2021 07:15
@wgzhao
Copy link
Member Author

wgzhao commented May 11, 2021 •
edited
Loading

@wgzhao looks like the CI failure is temporary, can just trigger another round of checks?

@jerryleooo All checks passed

@hashhar
Copy link
Member

hashhar commented May 11, 2021

@wgzhao Sorry I pushed on this branch instead by mistake - can you force push again. It looks good to merge % a couple of fixups (regarding testLimitPushdown)

@wgzhao
Copy link
Member Author

wgzhao commented May 11, 2021

@wgzhao Sorry I pushed on this branch instead by mistake - can you force push again. It looks good to merge % a couple of fixups (regarding testLimitPushdown)

Ok, I merge your updates and push again

@hashhar hashhar force-pushed the ck_support_aggregation_pushdown branch from ec244d9 to 4222d8e Compare May 11, 2021 08:55
@hashhar
Copy link
Member

hashhar commented May 11, 2021

Thanks @wgzhao . Please also rebase against latest master to get rid of the conflict.

@wgzhao wgzhao force-pushed the ck_support_aggregation_pushdown branch from 4222d8e to ae37521 Compare May 11, 2021 08:59
hashhar
hashhar approved these changes May 11, 2021
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good. Thanks a lot for the work on this and sorry for the delay.

Please squash into a single commit and remove the issue number from commit message like Implement aggregation pushdown for ClickHouse.

LGTM once CI finishes.

@wgzhao wgzhao force-pushed the ck_support_aggregation_pushdown branch 4 times, most recently from c85455b to 9a7484c Compare May 12, 2021 03:12
@wgzhao
Copy link
Member Author

wgzhao commented May 12, 2021

@hashhar pls check it

@hashhar
Copy link
Member

hashhar commented May 12, 2021

@wgzhao Thanks. I updated aggregation pushdown docs to point to https://trino.io/docs/current/optimizer/pushdown.html#aggregation-pushdown.

Will merge on CI.
Thanks for your work on this.

@hashhar hashhar force-pushed the ck_support_aggregation_pushdown branch from 9a7484c to dd13834 Compare May 12, 2021 09:27
@hashhar hashhar added this to the 357 milestone May 12, 2021
@hashhar hashhar merged commit cdb56d4 into trinodb:master May 12, 2021
@wgzhao wgzhao deleted the ck_support_aggregation_pushdown branch May 12, 2021 11:33
This was referenced May 12, 2021
Closed
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hashhar hashhar hashhar approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
357
Development

Successfully merging this pull request may close these issues.
3 participants
@wgzhao @jerryleooo @hashhar