Skip to content

Conversation

@JmPotato
Copy link
Contributor

Signed-off-by: JmPotato ghzpotato@gmail.com

Summary

  • Support the aggregation functions argMax and argMin.
  • Add test cases.

Changelog

  • New Feature

Related Issues

Close #466.

Test Plan

Unit Tests
Stateless Tests

Signed-off-by: JmPotato <ghzpotato@gmail.com>
@databend-bot databend-bot added the pr-feature this PR introduces a new feature to the codebase label May 27, 2021
@databend-bot
Copy link
Member

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

@JmPotato JmPotato mentioned this pull request May 27, 2021
Signed-off-by: JmPotato <ghzpotato@gmail.com>
@sundy-li
Copy link
Member

To add a stateless test is better, refer: ./scripts/ci/ci-stateless-tests-standalone.sh

@JmPotato
Copy link
Contributor Author

JmPotato commented May 27, 2021

To add a stateless test is better, refer: ./scripts/ci/ci-stateless-tests-standalone.sh

@sundy-li When I try to add a statement like:

SELECT argMax(a, b) from (select number as a, number as b from numbers_mt(10000) JOIN numbers_mt(10000));

Then an error occurs:

ERROR 1105 (HY000) at line 7: Code: 8, displayText = Unsupported function: "argMax".

I think these lines should be ok to register the two new functions, what do I miss?

impl AggregatorFunction {
    pub fn register(map: FactoryFuncRef) -> Result<()> {
        let mut map = map.write();
        map.insert("count", AggregateCountFunction::try_create);
        map.insert("min", AggregateMinFunction::try_create);
        map.insert("max", AggregateMaxFunction::try_create);
        map.insert("sum", AggregateSumFunction::try_create);
        map.insert("avg", AggregateAvgFunction::try_create);
        map.insert("argMin", AggregateArgMinFunction::try_create);
        map.insert("argMax", AggregateArgMaxFunction::try_create);
        Ok(())
    }
}

@zhang2014
Copy link
Member

FuseQuery always uses lowercase function names to get functions.

reference :
https://github.com/datafuselabs/datafuse/blob/master/common/functions/src/function_factory.rs#L40

@JmPotato
Copy link
Contributor Author

FuseQuery always uses lowercase function names to get functions.

reference :

https://github.com/datafuselabs/datafuse/blob/master/common/functions/src/function_factory.rs#L40

Oops, thanks for pointing out!

Signed-off-by: JmPotato <ghzpotato@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #642 (5c454e4) into master (c278fc1) will decrease coverage by 0%.
The diff coverage is 93%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #642    +/-   ##
=======================================
- Coverage      80%     80%    -1%     
=======================================
  Files         284     286     +2     
  Lines       13861   14070   +209     
=======================================
+ Hits        11160   11320   +160     
- Misses       2701    2750    +49     
Impacted Files Coverage Δ
common/datavalues/src/data_value_operator.rs 92% <0%> (-5%) ⬇️
common/datavalues/src/macros.rs 86% <ø> (ø)
common/datavalues/src/data_array_aggregate_test.rs 95% <96%> (+<1%) ⬆️
common/datavalues/src/data_array_aggregate.rs 79% <100%> (+4%) ⬆️
common/functions/src/expressions/cast.rs 65% <0%> (-14%) ⬇️
common/functions/src/comparisons/comparison.rs 77% <0%> (-12%) ⬇️
common/planners/src/plan_expression_test.rs 89% <0%> (-8%) ⬇️
common/functions/src/function_column.rs 28% <0%> (-7%) ⬇️
common/functions/src/arithmetics/arithmetic.rs 65% <0%> (-6%) ⬇️
common/functions/src/strings/substring.rs 66% <0%> (-5%) ⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c278fc1...5c454e4. Read the comment docs.

Copy link
Member

@sundy-li sundy-li left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution.

@databend-bot
Copy link
Member

CI Passed
Reviewer Approved
Let's Merge

@databend-bot databend-bot merged commit 6d143b2 into databendlabs:master May 27, 2021
@JmPotato JmPotato deleted the support_argMax_argMin branch May 27, 2021 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-feature this PR introduces a new feature to the codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support argMax/argMin

5 participants