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/parser: adjust the precedence of JSONB operators #41095

Merged
merged 1 commit into from
Sep 26, 2019

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Sep 25, 2019

Previously, JSONB operators had a precedence different from the one
that Postgres uses. This commit brings it more inline with the latter
and improves the compatibility.

All of the affected operators fall under '(any other operator)'
category in
https://www.postgresql.org/docs/12/sql-syntax-lexical.html#SQL-PRECEDENCE.

Fixes: #40855.

Release justification: Category 4: Low risk, high benefit changes to
existing functionality.

Release note: None

@yuzefovich yuzefovich requested review from jordanlewis, rafiss and a team September 25, 2019 20:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich
Copy link
Member Author

I'm not sure whether we should merge this before the 19.2 branch is cut.

@rohany
Copy link
Contributor

rohany commented Sep 25, 2019

I don't have an opinion on merging it or not, but the changes look good to me -- it was good to follow the postgres precedence table. Can you also add tests for the other operators who's precedence has changed (or do you think that is necessary) ?

@jordanlewis
Copy link
Member

This feels like a pretty scary thing to merge at the last second. It'll break compatibility with some queries in theory no matter when we merge it, but at this point I'd rather wait til next release if you don't mind.

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

awesome change!

Possibly, this commit should also adjust the PGJDBC expected failiures, look for org.postgresql.test.jdbc4.JsonbTest. in pkg/cmd/roachtest/pgjdbc_blacklist.go

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @rafiss)

@jordanlewis
Copy link
Member

I'd be open to being convinced that this should go into 19.2 if you explain which operators would see a material change, and if there's some real customer problem we're solving wiith this.

@yuzefovich yuzefovich changed the title sql/parser: adjust the precedence of a few operators sql/parser: adjust the precedence of JSONB operators Sep 25, 2019
@yuzefovich
Copy link
Member Author

I added parse tests for all operators in question (and removed the change to ~ operator). I don't see a reason to merge it now other than it would improve the compatibility. At the same time, I think it's unlikely to break anyone's code (unless someone is relying on the current difference between CockroachDB and PG), and I feel like users who ran into this problem before were likely to add extra parenthesis themselves, and those queries will not be affected.

I'll look into pgjdbc tests shortly.

Previously, JSONB operators had a precedence different from the one
that Postgres uses. This commit brings it more inline with the latter
and improves the compatibility.

All of the affected operators fall under '(any other operator)'
category in
https://www.postgresql.org/docs/12/sql-syntax-lexical.html#SQL-PRECEDENCE.

Release justification: Category 4: Low risk, high benefit changes to
existing functionality.

Release note: None
@yuzefovich
Copy link
Member Author

I confirmed that this change allows for two PGJDBC tests to pass, and I updated the 19.2 blacklist (assuming that we'll merge this). AFAIU, we will not be backporting this PR onto previous releases, so the blacklists for them should remain unchanged, right?

@jordanlewis
Copy link
Member

Yes, I don't think we'll backport this. Upon inspection, I no longer think this is too risky to merge. I think in fact it would be nice to get this into 19.2 - it may well prevent some annoying compatibility issues for real people out there.

@yuzefovich
Copy link
Member Author

TFTRs!

bors r=jordanlewis

craig bot pushed a commit that referenced this pull request Sep 26, 2019
41095: sql/parser: adjust the precedence of JSONB operators r=jordanlewis a=yuzefovich

Previously, JSONB operators had a precedence different from the one
that Postgres uses. This commit brings it more inline with the latter
and improves the compatibility.

All of the affected operators fall under '(any other operator)'
category in
https://www.postgresql.org/docs/12/sql-syntax-lexical.html#SQL-PRECEDENCE.

Fixes: #40855.

Release justification: Category 4: Low risk, high benefit changes to
existing functionality.

Release note: None

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Sep 26, 2019

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: fix associativity of jsonb ? operator to check if key exists
5 participants