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

sql: support regr_avgx() #57295

Merged
merged 1 commit into from
Dec 4, 2020
Merged

sql: support regr_avgx() #57295

merged 1 commit into from
Dec 4, 2020

Conversation

obahareth
Copy link
Contributor

Support aggregate function regr_avgx(). It referred to the PostgreSQL implementation, but the implementation details follow SQL 2003. See: postgresql.org/docs/10/functions-aggregate.html

See #41274

@obahareth obahareth requested a review from a team as a code owner December 1, 2020 14:47
@blathers-crl
Copy link

blathers-crl bot commented Dec 1, 2020

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Dec 1, 2020
@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented Dec 1, 2020

CLA assistant check
All committers have signed the CLA.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@obahareth
Copy link
Contributor Author

I'm fixing the conflicts and pushing a better version of the tests.

@blathers-crl
Copy link

blathers-crl bot commented Dec 1, 2020

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@yuzefovich
Copy link
Member

Hey @obahareth, thanks for pushing this PR out! Is this ready for review?

@blathers-crl
Copy link

blathers-crl bot commented Dec 2, 2020

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@obahareth
Copy link
Contributor Author

Hey @yuzefovich, I should have fixed the failing tests/lint issues with this recent push. I think we should be good to go.

How should I handle the conflict with pkg/sql/execinfrapb/processors_sql.pb.go seeing as it's a generated file?

@blathers-crl
Copy link

blathers-crl bot commented Dec 2, 2020

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

You need to regenerate those files and include the changes to your commit (I usually do make buildshort, but I think we also have make protobuf).

nit: we try to keep the commit messages with 80 character limit per line.
nit: the commit message should also contain a release note, something like

Release note (sql change): Aggregate function `regr_avgx` is now supported.

The code looks good to me with minor nits. We also should add some unit tests in aggregate_builtins_test.go similar to what we do for other statistics aggregates.

Reviewed 10 of 10 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @obahareth)


pkg/sql/distsql/columnar_operators_test.go, line 75 at r1 (raw file):

	execinfrapb.AggregatorSpec_COVAR_POP:            2,
	execinfrapb.AggregatorSpec_COVAR_SAMP:           2,
	execinfrapb.AggregatorSpec_REGR_AVGX:            2,

nit: add it as the last entry so that the order is the same as in processors_sql.proto.


pkg/sql/sem/builtins/aggregate_builtins.go, line 2021 at r1 (raw file):

}

// regressionAvgXAggregate represents SQL:2003 average of the independent variable (sum(X)/N)

nit: we try to keep comments also at 80 character limit, and a period is missing.

@blathers-crl
Copy link

blathers-crl bot commented Dec 4, 2020

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

Support aggregate function `regr_avgx()`. It referred to the PostgreSQL
implementation, but the implementation details follow SQL 2003.
See: postgresql.org/docs/10/functions-aggregate.html

Release note (sql change): Aggregate function `regr_avgx` is now supported.

Co-authored-by: Saif Al-Harthi <[email protected]>
@obahareth
Copy link
Contributor Author

There we go @yuzefovich, I believe I took care of all the comments you had, please let me know if I missed anything.

Copy link
Member

@yuzefovich yuzefovich 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!

Note: the CI is red due to a flake.

bors r=yuzefovich

Reviewed 5 of 5 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@craig
Copy link
Contributor

craig bot commented Dec 4, 2020

Build succeeded:

@craig craig bot merged commit 826ab33 into cockroachdb:master Dec 4, 2020
@SWDevAngel
Copy link

@obahareth Hi Omar. Thank you for contributing to CockroachDB this year. As a token of our appreciation we would like to send you a gift. Please DM me on our community Slack @lisa so I can send you a link. (If you don’t want a gift, we also have a charitable contribution choice.) All orders need to be in by December 13, so please send this asap. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants