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

*: add builtin aggregate function var_pop #11155

Closed
wants to merge 3,147 commits into from
Closed

Conversation

githubFZX
Copy link
Contributor

@githubFZX githubFZX commented Jul 9, 2019

What problem does this PR solve?

#7623

Tests

mysql> create table numerical_t(
    -> a tinyint,
    -> b smallint,
    -> c mediumint,
    -> d int,
    -> e bigint,
    -> f float,
    -> g double,
    -> h decimal
    -> );
Query OK, 0 rows affected (0.10 sec)

mysql> insert into numerical_t 
    -> values
    -> (1, 2, 3, 4, 5, 6.1, 7.2, 8.3), 
    -> (2, 3, 4, 5, 6, 7.2, 8.3, 9.4),
    -> (1, 3, 4, 5, 6, 7.1, 8.2, 9.3);
Query OK, 3 rows affected, 3 warnings (0.01 sec)

mysql> select a, var_pop(b),var_pop(c),var_pop(d),var_pop(e),var_pop(f),var_pop(g),var_pop(h) from numerical_t group by a;
+------+------------+------------+------------+------------+------------+--------------------+------------+
| a    | var_pop(b) | var_pop(c) | var_pop(d) | var_pop(e) | var_pop(f) | var_pop(g)         | var_pop(h) |
+------+------------+------------+------------+------------+------------+--------------------+------------+
|    1 |       0.25 |       0.25 |       0.25 |       0.25 |       0.25 | 0.2500000000000071 |       0.25 |
|    2 |          0 |          0 |          0 |          0 |          0 |                  0 |          0 |
+------+------------+------------+------------+------------+------------+--------------------+------------+
2 rows in set (0.00 sec)

@CLAassistant
Copy link

CLAassistant commented Jul 9, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
19 out of 21 committers have signed the CLA.

✅ lonng
✅ crazycs520
✅ qw4990
✅ foreyes
✅ tiancaiamao
✅ lysu
✅ jackysp
✅ zz-jason
✅ sticnarf
✅ SunRunAway
✅ eurekaka
✅ disksing
✅ EmilStenstrom
✅ wshwsh12
✅ Reminiscent
✅ WangXiangUSTC
✅ lamxTyler
✅ ZYunH
✅ jiyfhust
❌ root
❌ githubFZX


root seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

zhaoyanxing and others added 17 commits July 15, 2019 11:33
Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
…11032)

Signed-off-by: MyonKeminta <MyonKeminta@users.noreply.github.com>
@zz-jason
Copy link
Member

@githubFZX Thanks for your contribution, please follow the Contribution Guide to add a proper PR title.

@zz-jason zz-jason added contribution This PR is from a community contributor. sig/execution SIG execution labels Jul 17, 2019
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@githubFZX
Copy link
Contributor Author

githubFZX commented Jul 17, 2019 via email

@zz-jason
Copy link
Member

zz-jason commented Sep 1, 2019

/run-all-tests

@zz-jason
Copy link
Member

zz-jason commented Sep 1, 2019

@githubFZX please sign the CLA. Ref to #11155 (comment)

@@ -344,11 +352,11 @@ func (s *testSuite1) TestAggregation(c *C) {
_, err = tk.Exec("select std_samp(a) from t")
// TODO: Fix this error message.
c.Assert(errors.Cause(err).Error(), Equals, "[expression:1305]FUNCTION std_samp does not exist")
_, err = tk.Exec("select variance(a) from t")
//_, err = tk.Exec("select variance(a) from t")
//c.Assert(errors.Cause(err).Error(), Equals, "unsupported agg function: variance")
Copy link
Member

Choose a reason for hiding this comment

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

please do not comment them out, you can simply rewrite the expected behavior of this test.

_, err = tk.Exec("select var_pop(a) from t")
c.Assert(errors.Cause(err).Error(), Equals, "unsupported agg function: var_pop")
//_, err = tk.Exec("select var_pop(a) from t")
//c.Assert(errors.Cause(err).Error(), Equals, "unsupported agg function: var_pop")
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@@ -233,6 +235,12 @@ func (a *baseFuncDesc) typeInfer4LeadLag(ctx sessionctx.Context) {
}
}

func (a *baseFuncDesc) typeInfer4VarPop(ctx sessionctx.Context) {
//var_pop's return value type is double
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//var_pop's return value type is double
// var_pop's return value type is double

It's recommended to add a space between // and the first word.

@zz-jason zz-jason changed the title Add var_pop *: add builtin aggregate function var_pop Sep 1, 2019
baseAggFunc
}

type partialResult4Float64 struct {
Copy link
Member

Choose a reason for hiding this comment

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

It's better to use the algorithm described by Chan, Golub, and LeVeque in "Algorithms for computing the sample variance: analysis and recommendations" to calculate the variance. You can find how apache hive implement this at here: https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDAFVariance.java#L194

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I will refer to it.

@zz-jason
Copy link
Member

zz-jason commented Nov 5, 2019

@githubFZX friendly ping, any update?

@githubFZX githubFZX requested review from a team as code owners December 3, 2019 12:04
@ghost ghost requested review from wshwsh12, XuHuaiyu, alivxxx and lzmhhh123 and removed request for a team December 3, 2019 12:04
@lzmhhh123 lzmhhh123 closed this Dec 3, 2019
@zz-jason zz-jason reopened this Dec 11, 2019
@alivxxx alivxxx removed their request for review December 12, 2019 04:34
@lzmhhh123 lzmhhh123 removed their request for review December 12, 2019 07:07
@githubFZX githubFZX closed this Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution This PR is from a community contributor. sig/execution SIG execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.