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: add square and cube root unary operators (|/ and ||/) #47680

Merged
merged 1 commit into from
Apr 22, 2020

Conversation

hueypark
Copy link
Contributor

Fixes #47544

Release note (sql change): add square and cube root unary operators (|/ and ||/)

@hueypark hueypark requested a review from a team as a code owner April 19, 2020 12:57
@blathers-crl
Copy link

blathers-crl bot commented Apr 19, 2020

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

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.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm:

CC @yuzefovich if you want to take a quick look as well

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

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.

Very nice work! :lgtm: if edge cases (NULLs and NaNs) are handled correctly.

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


pkg/sql/logictest/testdata/logic_test/operator, line 7 at r1 (raw file):


query error cannot take square root of a negative number
SELECT |/ -1.0::float

I'd be curious to see test cases with NULLs and NaNs (not sure if those are applicable here though).

nit: I think this test belongs in sem/tree/eval folder.

@blathers-crl
Copy link

blathers-crl bot commented Apr 21, 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.

Copy link
Contributor Author

@hueypark hueypark left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @yuzefovich)


pkg/sql/logictest/testdata/logic_test/operator, line 7 at r1 (raw file):

Previously, yuzefovich wrote…

I'd be curious to see test cases with NULLs and NaNs (not sure if those are applicable here though).

nit: I think this test belongs in sem/tree/eval folder.

Done.

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.

Thanks for your contribution!

bors r=raduberinde,yuzefovich

Reviewed 2 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 2 stale)

@craig
Copy link
Contributor

craig bot commented Apr 21, 2020

Build failed (retrying...)

@craig
Copy link
Contributor

craig bot commented Apr 21, 2020

Build failed (retrying...)

@craig
Copy link
Contributor

craig bot commented Apr 21, 2020

Build failed (retrying...)

@craig
Copy link
Contributor

craig bot commented Apr 21, 2020

Build failed

@yuzefovich
Copy link
Member

Hey @hueypark, do you mind rebasing your PR on top of the current master? We have a fix for some infrastructure problems in there that we need to pick up before merging this.

Fixes cockroachdb#47544

Release note (sql change): add square and cube root unary operators (|/ and ||/)
@hueypark
Copy link
Contributor Author

@yuzefovich Done!

@blathers-crl
Copy link

blathers-crl bot commented Apr 22, 2020

❌ The GitHub CI (Cockroach) build has failed on 96193f1a.

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

@yuzefovich
Copy link
Member

Thanks!

The build failure seems like a flake to me.

bors r=raduberinde,yuzefovich

@craig
Copy link
Contributor

craig bot commented Apr 22, 2020

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: implement square and cube root unary operators (|/ and ||/)
4 participants