cockroachdb / cockroach

CockroachDB — the cloud native, distributed SQL database designed for high availability, effortless scale, and control over data placement.
https://www.cockroachlabs.com
Other
29.89k stars 3.78k forks source link

sql: operator precedence does not match PostgreSQL #66866

Open mikecaat opened 3 years ago

mikecaat commented 3 years ago

Unfortunately, CockroachDB does not match PostgreSQL's order of operations. An example of this is here: https://github.com/cockroachdb/cockroach/pull/64699#issuecomment-832403559 and is detailed here: https://github.com/cockroachdb/cockroach/issues/66866#issuecomment-868359049

Changing the order of operations is dangerous and will be very broken for backwards compatibility. However, if we want PostgreSQL compatibility we need to do it. One for the record books...


Describe the problem

When I made a test for #66861, I found the order of operation of OPERATOR(pg_config.X) is not same.

root@localhost:26257/postgres> SELECT 'a' || 'b' = 'ab';
  ?column?
------------
    true
(1 row)

root@localhost:26257/postgres> SELECT 'a' || 'b' OPERATOR(pg_catalog.=) 'ab';
  ?column?
------------
  afalse
(1 row)

By the way, the result of executing the same queries with postgresql ver 13.3 is the following.

psql=# SELECT 'a' || 'b' = 'ab';
 ?column?
----------
 t
(1 row)

psql=# SELECT 'a' || 'b' OPERATOR(pg_catalog.=) 'ab';
 ?column?
----------
 t
(1 row)

To Reproduce

Execute the above example.

Expected behavior

I think this should be the same order of operations. Thought?

Environment:

Jira issue: CRDB-8268

blathers-crl[bot] commented 3 years ago

Hello, I am Blathers. I am here to help you get the issue triaged.

Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here.

I have CC'd a few people who may be able to assist you:

If we have not gotten back to your issue within a few business days, you can try the following:

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

rafiss commented 3 years ago

Thanks for the report! cc @otan do you happen to know what Postgres does for the precedence of OPERATOR?

otan commented 3 years ago

Sorry, my previous comment was wrong.

The order of precedence is defined here: https://github.com/cockroachdb/cockroach/blob/b971e36477d7aa19c39d2272662ae8cf061ff2ba/pkg/sql/parser/sql.y#L1339

Compared to PostgreSQL: https://github.com/postgres/postgres/blob/9b8ed0f52bffceaccf6fa650ffe482e7da20fbf2/src/backend/parser/gram.y#L796

With || tokenised as CONCAT. In PostgreSQL, %left Op should include CONCAT, so i'm not quite sure what precedence is broken here.

maybe the answer is to do https://github.com/cockroachdb/cockroach/pull/65107 for BinaryExprs and ComparisonExprs, since it may pretty print the AST differently.

mikecaat commented 3 years ago

Hi, thanks for your comments!

I investigated a little more. On the second thought, I think this is not a bug, but CRDB's limitation of PostgreSQL compatibility.

PostgreSQL's operator precedence said that operators including OPERATOR, CONCAT, &, #, | and so on is the same. But, CRDB doesn't follow PostgreSQL's way intentionally.

A example case is the follow,

In PostgreSQL, the operators | (bitwise OR), # (bitwise XOR), and & (bitwise AND) all have the same precedence. In CockroachDB, the precedence from highest to lowest is: &, #, |.

So, the issue I addressed happens with other operator combinations. That's why I think this is not a bug if this behavior is expected. Out of curiosity, I want to know why the behavior is implemented purposely.

Though?

otan commented 3 years ago

I've had a go at changing the order of precedence, but the breaking of compatibility is too hard.

I'm going to rename/reword your issue

mikecaat commented 3 years ago

OK, thanks.