Changes by lincolnq (@lincolnq):

• edited description

Changes by lincolnq (@lincolnq):

• edited description

lincolnq (@lincolnq) wrote:

I noticed that commit a80bb4e changed the precedence of -> from 5 to 15, which seems like it would explain the problem.

Michael Bayer (@zzzeek) wrote:

If I change it to 5 I get this:

        self.assert_compile(
            col[col2 + 8],
            "x -> (y + :y_1)",
            checkparams={'y_1': 8}
        )

fails with:

x -> y + :y_1

which is a disaster.

I don't have a solution at the moment and I have a great fear that the precedence rules here are not backend-agnostic, meaning the whole refactor would have to be done again.

Changes by Michael Bayer (@zzzeek):

• added labels: sql

Changes by Michael Bayer (@zzzeek):

• set milestone to "1.1"

Michael Bayer (@zzzeek) wrote:

uh but if I change precedence to '5' your test fails anyway. so...that's not the problem.

Michael Bayer (@zzzeek) wrote:

oh, because HSTORE is separate.

Michael Bayer (@zzzeek) wrote:

proposed fix at https://gerrit.sqlalchemy.org/197

lincolnq (@lincolnq) wrote:

I think your fix is probably the right direction -- Sqlalchemy should output extra parentheses for safety and this seems like the right way to do it (though I don't know the code at all so I can't comment on most of it).

This bug's existence implies a whole class of similar operator-precedence bugs on old Postgres (many having nothing to do with HSTORE/JSON) -- it looks like you only implemented the change for HSTORE and JSON, right? It seems like it would be worth making sure all operators which changed precedence between these two versions have the eager_grouping enabled.

https://www.postgresql.org/docs/9.4/static/sql-syntax-lexical.html#SQL-PRECEDENCE-TABLE
https://www.postgresql.org/docs/9.5/static/sql-syntax-lexical.html#SQL-PRECEDENCE-TABLE

Michael Bayer (@zzzeek) wrote:

it looks like they moved the precedence of "IS / IS NOT" to be lower. The use case of "a b <IS/ISNOT> (NULL/true/false)" is not that common outside of the json/hstore index operations, and in any case the issue here is a regression from 1.0.x specific to index operations. The solution I took here was just to hardwire extra parenthesization into these operators, the reason we are conservative about parenthesis otherwise is that you if you aren't you end up with things like "((a + b) / c) + ((((e + f) + g) + h ) + i)" which is difficult to work with and is often rejected by the database itself if the expression is in a less common place (not Postgresql, more like SQL Server, Oracle, SQLite etc. have had issues with unnecessary parenthesis causing syntax errors).

we've not had any issues reported from the change in precedence of IS/ISNOT from 9.4 to 9.5 across existing SQLAlchemy releases, those can be dealt with as they are reported.

Michael Bayer (@zzzeek) wrote:

Add "eager_parenthesis" late-compilation rule, use w/ PG JSON/HSTORE

Added compiler-level flags used by Postgresql to place additional
parenthesis than would normally be generated by precedence rules
around operations involving JSON, HSTORE indexing operators as well as
within their operands since it has been observed that Postgresql's
precedence rules for at least the HSTORE indexing operator is not
consistent between 9.4 and 9.5.

Fixes: #3806
Change-Id: I5899677b330595264543b055abd54f3c76bfabf2

→ 333414f

Changes by Michael Bayer (@zzzeek):

• changed status to closed