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

Push aggregate in LookupIndex #3504

Merged
merged 2 commits into from
Jan 10, 2022
Merged

Conversation

Nivras
Copy link
Contributor

@Nivras Nivras commented Dec 20, 2021

What type of PR is this?

  • bug
  • feature
  • enhancement

What does this PR do?

LookupIndexRequest can receive the statProps info, and do aggregate in storage.

This PR add a new node(IndexAggregateNode) for LookupIndexPlan, it will check and append the columns which in statProp but not in returnColumns, and append it in returnColumns.

IndexProjectNode before this PR will return the row in the order of returnColumns, after this PR, it will return returnColumns + statProps(which the statProp not in returnColumns). and IndexAggregateNode will truncate the Row, only real returnColumns append into resultDataSet, and calculate the aggregate expression and return the result.

Which issue(s)/PR(s) this PR relates to?

Special notes for your reviewer, ex. impact of this fix, etc:

Additional context/ Design document:

Checklist:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the corresponding label.)
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Please confirm whether to be reflected in release notes and how to describe:

                                                            `

@Nivras Nivras added do not review PR: not ready for the code review yet ready-for-testing PR: ready for the CI test and removed do not review PR: not ready for the code review yet labels Dec 20, 2021
@Nivras Nivras changed the title Push index down Push aggregate in LookupIndex Dec 23, 2021
@Nivras Nivras force-pushed the push_index_down branch 4 times, most recently from cf00464 to 51a87f2 Compare December 29, 2021 02:39
Copy link
Contributor

@critical27 critical27 left a comment

Choose a reason for hiding this comment

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

Goood job~~ Generally LGTM, clean code

src/interface/storage.thrift Show resolved Hide resolved
src/storage/exec/IndexAggregateNode.cpp Show resolved Hide resolved
critical27
critical27 previously approved these changes Jan 5, 2022
Copy link
Contributor

@critical27 critical27 left a comment

Choose a reason for hiding this comment

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

Good job~

cangfengzhs
cangfengzhs previously approved these changes Jan 6, 2022
Copy link
Contributor

@cangfengzhs cangfengzhs left a comment

Choose a reason for hiding this comment

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

LGTM

@critical27 critical27 merged commit 8cab6d0 into vesoft-inc:master Jan 10, 2022
@Sophie-Xie Sophie-Xie added the doc affected PR: improvements or additions to documentation label Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc affected PR: improvements or additions to documentation ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants