Skip to content

sql: remove ~ in favor of bitwise_not()#14908

Merged
madelynnblue merged 1 commit intocockroachdb:masterfrom
madelynnblue:tilde-func
Apr 14, 2017
Merged

sql: remove ~ in favor of bitwise_not()#14908
madelynnblue merged 1 commit intocockroachdb:masterfrom
madelynnblue:tilde-func

Conversation

@madelynnblue
Copy link
Copy Markdown
Contributor

~ is troublesome because of its precedence. In Postgres, it has
an (unexpected) fairly low precedence (see #14877). However in
Spanner it has the expected high precedence of a unary operator
(https://cloud.google.com/spanner/docs/functions-and-operators#operators).
This is problematic because we want to support Postgres SQL correctly,
but also have things make sense. In this case we feel that the Postgres
operator precedence is incorrect. But in order to not return silently
incorrect results that depend on this behavior, we are replacing the
operator with an equivalent function. This will cause applications
to explicitly error so users can fix them correctly.

See discussion in #14814

@madelynnblue madelynnblue requested review from jordanlewis and knz April 13, 2017 19:51
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@madelynnblue
Copy link
Copy Markdown
Contributor Author

After doing all the work to make this, I now dislike it a lot because of all the stuff it removed. I'm unsure how to proceed.

@jordanlewis
Copy link
Copy Markdown
Member

All of this removal seems like a positive thing to me, but if it makes you anxious, I suppose you could just change the old implementation to raise an "unimplemented" error on evaluation.

@knz
Copy link
Copy Markdown
Contributor

knz commented Apr 14, 2017

I concur with Jordan, this makes the code more simple.
The change to add it back is relatively trivial so I am not worried about the future.
LGTM


Reviewed 12 of 12 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

~ is troublesome because of its precedence. In Postgres, it has
an (unexpected) fairly low precedence (see #14877). However in
Spanner it has the expected high precedence of a unary operator
(https://cloud.google.com/spanner/docs/functions-and-operators#operators).
This is problematic because we want to support Postgres SQL correctly,
but also have things make sense. In this case we feel that the Postgres
operator precedence is incorrect. But in order to not return silently
incorrect results that depend on this behavior, we are replacing the
operator with an equivalent function. This will cause applications
to explicitly error so users can fix them correctly.

See discussion in #14814
@madelynnblue madelynnblue merged commit de674dc into cockroachdb:master Apr 14, 2017
@madelynnblue madelynnblue deleted the tilde-func branch April 14, 2017 19:23
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.

4 participants