Operator precedence and parentheses in queries

[roji]

I have an issue with test QueryTestBase.String_Compare_nested. The test generates the following query:

SELECT "c"."CustomerID", "c"."Address", "c"."City", "c"."CompanyName", "c"."ContactName", "c"."ContactTitle", "c"."Country", "c"."Fax", "c"."Phone", "c"."PostalCode", "c"."Region"
        FROM "Customers" AS "c"
        WHERE "c"."CustomerID" <= 'M' || "c"."CustomerID"

However, it seems that in PostgreSQL, the string concatenation operator || has lower priority than the comparison operator <=, and so the WHERE clause gets a string instead of a bool.

IMHO this is very strange behavior on PostgreSQL's side - have sent a message to their dev list. However, it raises the larger question of operator precedence and parentheses. I can see some basic logic for that in DefaultQuerySqlGenerator.VisitBinary but in my case both sides of the expression are simple. I am also looking into mapping various C# operations to PostgreSQL-specific operators (regex, json), and the problem might pop up there as well.

A full, complete solution would assign an SQL priority to each expression, take that into account when generating parenthese and allow providers to override. This would be a non-trivial change I guess.

A quicker hack would be for providers to simply allow providers to override a new RequiresParentheses method, which would accept an Expression and would return whether to always surround its SQL with parentheses. If you want to go down that route I can submit a PR.

[rowanmiller]

@anpete what are your thoughts on this one?

[anpete]

@roji Can you override VisitBinary in your SQL generator to add the parens?

[roji]

Yep, no problem - but it would mean duplicating the entire VisitBinary code into Npgsql, since there's no narrower extension point. I've sent PR #3065 which adds the RequiresParentheses for this. This is the "quick hack" approach which I mentioned above.

However, it doesn't really tackle the entire problem - operators vary between databases and precedence does too (PostgreSQL even allows you to define user operators). So when deciding whether to put parentheses it seems like a more complete system needs to be in place.

I do realize that would be a big task and I don't necessary see a burning need right now, so the PR I sent is definitely fine for now - but maybe it's worth thinking about the larger problem (and leaving this issue open) for later?

[anpete]

What about overriding VisitBinary and detecting the string concatenation (NodeType == Add and operand types == string) so you can output parens around a call to base.

[roji]

That would work too, should have thought of it myself.

Should I keep this issue open for the larger problem?

[anpete]

I think we will try our best to not try and do the complex thing :-) so I think we can close for now.