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

feat(frontend): support filter clause in aggregation #3626

Merged
merged 16 commits into from
Jul 5, 2022

Conversation

Enter-tainer
Copy link
Contributor

@Enter-tainer Enter-tainer commented Jul 4, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

Summarize your change

This PR adds selective aggregation support to the frontend.

How does this PR work?

To parse the selective aggregation syntax, the parser, the binder, and the logical_agg plan node have been changed.

  • The column prune of LogicalAgg is changed as well to preserve columns that are referred to by the filter clause.
  • When performing 2 phase aggregation, only the partial_agg needs filter.

Limitations of the Current Code

  1. Since the parser and the AST is changed, sqlsmith is changed as well. But currently, it can only generate aggregation functions without any filter.
  2. An extra field is_in_filter_clause is added in LogicalAggBuilder because the filter clause can refer to any columns while other places can only refer to group keys. So I add this field to distinguish between them. IMO the current implementation is kind of dirty.
  • Add the 'user-facing changes' label if your PR contains changes that are visible to users (optional)

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

TODO

  • add more tests & check corner cases
  • filter clause should be taken into account in rewrite_with_input

Refer to a related PR or issue link (optional)

Tracking in #3589, RFC #3506

Future works

After adding selective aggregation in the frontend, I will continue working to add this function to the executors.

@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #3626 (37cf174) into main (ce7612d) will increase coverage by 0.02%.
The diff coverage is 92.74%.

@@            Coverage Diff             @@
##             main    #3626      +/-   ##
==========================================
+ Coverage   74.38%   74.40%   +0.02%     
==========================================
  Files         781      781              
  Lines      110682   110786     +104     
==========================================
+ Hits        82330    82433     +103     
- Misses      28352    28353       +1     
Flag Coverage Δ
rust 74.40% <92.74%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/frontend/src/expr/expr_rewriter.rs 72.50% <0.00%> (-5.88%) ⬇️
src/frontend/src/expr/mod.rs 85.32% <0.00%> (-0.67%) ⬇️
src/tests/sqlsmith/src/expr.rs 89.47% <ø> (ø)
src/frontend/src/expr/agg_call.rs 57.60% <90.00%> (+2.31%) ⬆️
src/frontend/src/binder/expr/function.rs 85.95% <100.00%> (+2.96%) ⬆️
...rc/frontend/src/optimizer/plan_node/logical_agg.rs 95.34% <100.00%> (+0.29%) ⬆️
src/frontend/src/utils/condition.rs 96.45% <100.00%> (+0.27%) ⬆️
src/sqlparser/src/ast/mod.rs 89.51% <100.00%> (+0.03%) ⬆️
src/sqlparser/src/parser.rs 92.20% <100.00%> (+0.02%) ⬆️
src/meta/src/manager/id.rs 94.94% <0.00%> (-0.57%) ⬇️
... and 6 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@Enter-tainer Enter-tainer added the user-facing-changes Contains changes that are visible to users label Jul 5, 2022
@Enter-tainer Enter-tainer marked this pull request as ready for review July 5, 2022 01:10
@Enter-tainer Enter-tainer changed the title feat(frontend): support filter clause in aggregation [WIP] feat(frontend): support filter clause in aggregation Jul 5, 2022
st1page
st1page previously approved these changes Jul 5, 2022
Copy link
Contributor

@st1page st1page left a comment

Choose a reason for hiding this comment

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

generally LGTM. the planner tests are comprehensive, good work!

@st1page st1page dismissed their stale review July 5, 2022 02:55

some issue on the agg builder

Copy link
Contributor

@likg227 likg227 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@st1page st1page left a comment

Choose a reason for hiding this comment

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

should check if exist aggregation in the filter clause.
test on PG:

dev=# explain select max(v2) filter(where min(v3) > 1.0) from test group by v1;
ERROR:  aggregate functions are not allowed in FILTER
LINE 1: explain select max(v2) filter(where min(v3) > 1.0) from test...

Copy link
Contributor

@st1page st1page left a comment

Choose a reason for hiding this comment

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

LGTM

@Enter-tainer Enter-tainer added mergify/can-merge Indicates that the PR can be added to the merge queue component/frontend Protocol, parsing, binder. labels Jul 5, 2022
@mergify mergify bot merged commit 0d7d9a9 into main Jul 5, 2022
@mergify mergify bot deleted the lwz/selective-agg-frontend branch July 5, 2022 07:20
nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this pull request Aug 9, 2022
…3626)

* parse filter in agg

* bind selective agg

* fix filter clause input ref index

* fix logical agg column prune

* modify parser test, add filter

* fmt code

* remove unused struct

* add more tests and error reporting

* add test for non-agg filter clause

* add group by test

* fix `rewrite_with_input`

* fix 2 phase agg

* report error for agg calls in the filter clause

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/frontend Protocol, parsing, binder. mergify/can-merge Indicates that the PR can be added to the merge queue type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants