[Vectorized][Feature] Support min_by/max_by function. #8623

zhannngchen · 2022-03-24T06:30:39Z

Proposed changes

Issue Number: open #7678

Problem Summary:

Support min_by/max_by on vectorized engine.

Checklist(Required)

Does it affect the original behavior: (No)
Has unit tests been added: (Yes)
Has document been added or modified: (Yes)
Does it need to update dependencies: (No)
Are there any changes that cannot be rolled back: (No)

Further comments

This is an initial patch, I will add the document later.
For now, aggregate functions only supports 1 intermediate type, but min_by/max_by needs 2 type information, I've did a tricky work-around in this initial patch, but we need to discuss that do we really need to change the intermediate type to an intermediate type list? @morningman @HappenLee @dataroaring. Another solution is to serialize the type information in the intermediate data.
To support these 2 functions in the non-vectorized code needs to pay much more efforts, we may need to do some refactor. Do we need to support this?

zhannngchen · 2022-03-24T06:41:01Z

fe/fe-core/src/main/java/org/apache/doris/analysis/AggregateInfo.java

-            Expr aggExprParam =
-                    new SlotRef(inputDesc.getSlots().get(i + getGroupingExprs().size()));
+            List<Expr> paramExprs = new ArrayList<>();
+            if (inputExpr.fn.functionName().equals("max_by")) {


It's a tricky workaround, won't merge to trunk code.

Discussed with @HappenLee and @yiguolei , we should change the intermediateType to a list, in another PR, in current PR, we just keep the hard code way.

zbtzbtzbt · 2022-03-24T06:54:47Z

we can add docs for these new functions @zhannngchen

zhannngchen · 2022-03-24T06:58:36Z

we can add docs for these new functions @zhannngchen

Yes, I'll update the patch later today

yiguolei · 2022-04-16T05:09:24Z

be/test/vec/aggregate_functions/agg_min_max_by_test.cpp

+                         ::testing::ValuesIn(std::vector<std::string> {"min_by", "max_by"}));
+} // namespace doris::vectorized
+
+int main(int argc, char** argv) {


We changed unit test running mechanism, not every unit test has a main function.

morningman · 2022-04-16T05:50:20Z

be/src/vec/aggregate_functions/aggregate_function_min_max_by.h

+        if (StringRef(Data::name()) == StringRef("min_by") ||
+            StringRef(Data::name()) == StringRef("max_by")) {
+            if (!key_type_->is_comparable()) {
+                LOG(FATAL) << fmt::format(


This should be an assertion?

yiguolei

LGTM

github-actions · 2022-04-16T06:16:22Z

PR approved by anyone and no changes requested.

morningman

LGTM

github-actions · 2022-04-18T09:26:43Z

PR approved by at least one committer and no changes requested.

Support min_by/max_by on vectorized engine.

…he#8623) Support min_by/max_by on vectorized engine.

morningman added kind/feature Categorizes issue or PR as related to a new feature. dev/backlog waiting to be merged in future dev branch labels Mar 24, 2022

zhannngchen commented Mar 24, 2022

View reviewed changes

zhannngchen changed the title ~~[Vectorized][New Feature] Support min_by/max_by function.~~ [Vectorized][Feature] Support min_by/max_by function. Mar 24, 2022

zhannngchen force-pushed the zc-max-by branch from 4d79e86 to e8cb28a Compare April 13, 2022 07:02

github-actions bot added area/planner Issues or PRs related to the query planner area/vectorization kind/docs Categorizes issue or PR as related to documentation. labels Apr 13, 2022

hf200012 requested a review from HappenLee April 16, 2022 03:15

shushi.zc and others added 6 commits April 16, 2022 11:57

draft

9fd1773

update

ce23150

add functionset

5b04c4c

update

45a040f

add documents

506ccfd

update

a62ef15

zhannngchen force-pushed the zc-max-by branch from e8cb28a to a62ef15 Compare April 16, 2022 03:59

yiguolei reviewed Apr 16, 2022

View reviewed changes

morningman reviewed Apr 16, 2022

View reviewed changes

yiguolei previously approved these changes Apr 16, 2022

View reviewed changes

yiguolei added the approved Indicates a PR has been approved by one committer. label Apr 16, 2022

github-actions bot added the reviewed label Apr 16, 2022

update

4b1e154

zhannngchen dismissed yiguolei’s stale review via 4b1e154 April 16, 2022 12:13

github-actions bot removed the approved Indicates a PR has been approved by one committer. label Apr 16, 2022

update

98dd88c

morningman approved these changes Apr 18, 2022

View reviewed changes

github-actions bot added the approved Indicates a PR has been approved by one committer. label Apr 18, 2022

morningman merged commit 1b4cd76 into apache:master Apr 20, 2022

morningman added dev/1.0.1-deprecated should be merged into dev-1.0.1 branch and removed dev/backlog waiting to be merged in future dev branch labels Apr 20, 2022

morningman pushed a commit that referenced this pull request Apr 20, 2022

[feature](vectorized)(function) Support min_by/max_by function. (#8623)

5ce3ae5

Support min_by/max_by on vectorized engine.

morningman added dev/merged-1.0.1-deprecated PR has been merged into dev-1.0.1 and removed dev/1.0.1-deprecated should be merged into dev-1.0.1 branch labels Apr 20, 2022

weizhengte pushed a commit to weizhengte/incubator-doris that referenced this pull request Apr 22, 2022

[feature](vectorized)(function) Support min_by/max_by function. (apac…

73d876c

…he#8623) Support min_by/max_by on vectorized engine.

zhengshiJ pushed a commit to zhengshiJ/incubator-doris that referenced this pull request Apr 27, 2022

[feature](vectorized)(function) Support min_by/max_by function. (apac…

51e2107

…he#8623) Support min_by/max_by on vectorized engine.

starocean999 pushed a commit to starocean999/incubator-doris that referenced this pull request May 19, 2022

[feature](vectorized)(function) Support min_by/max_by function. (apac…

729abf2

…he#8623) Support min_by/max_by on vectorized engine.

englefly pushed a commit to englefly/incubator-doris that referenced this pull request May 23, 2022

[feature](vectorized)(function) Support min_by/max_by function. (apac…

24051d4

…he#8623) Support min_by/max_by on vectorized engine.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vectorized][Feature] Support min_by/max_by function. #8623

[Vectorized][Feature] Support min_by/max_by function. #8623

zhannngchen commented Mar 24, 2022 •

edited

Loading

zhannngchen Mar 24, 2022 •

edited

Loading

zhannngchen Apr 18, 2022

zbtzbtzbt commented Mar 24, 2022

zhannngchen commented Mar 24, 2022

yiguolei Apr 16, 2022

zhannngchen Apr 18, 2022

morningman Apr 16, 2022

zhannngchen Apr 18, 2022

yiguolei left a comment

github-actions bot commented Apr 16, 2022

morningman left a comment

github-actions bot commented Apr 18, 2022

[Vectorized][Feature] Support min_by/max_by function. #8623

[Vectorized][Feature] Support min_by/max_by function. #8623

Conversation

zhannngchen commented Mar 24, 2022 • edited Loading

Proposed changes

Problem Summary:

Checklist(Required)

Further comments

zhannngchen Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

zhannngchen Apr 18, 2022

Choose a reason for hiding this comment

zbtzbtzbt commented Mar 24, 2022

zhannngchen commented Mar 24, 2022

yiguolei Apr 16, 2022

Choose a reason for hiding this comment

zhannngchen Apr 18, 2022

Choose a reason for hiding this comment

morningman Apr 16, 2022

Choose a reason for hiding this comment

zhannngchen Apr 18, 2022

Choose a reason for hiding this comment

yiguolei left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 16, 2022

morningman left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 18, 2022

zhannngchen commented Mar 24, 2022 •

edited

Loading

zhannngchen Mar 24, 2022 •

edited

Loading