Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove parentheses by allowing providers to specify SQL operator precedence and associativity #27253

Closed
roji opened this issue Jan 22, 2022 · 8 comments
Assignees

Comments

@roji
Copy link
Member

roji commented Jan 22, 2022

We currently generate parentheses in SQL around most binary expression, LIKE etc. Allow providers to provide the exact precedence of operators/expressions, to remove unnecessary parentheses.

The default relational behavior would still be conservative - as today - and defensively add parentheses in many cases. This makes it an opt-in provider feature.

@ErikEJ
Copy link
Contributor

ErikEJ commented Jan 22, 2022

Would that mainly be about aesthetics?

@roji
Copy link
Member Author

roji commented Jan 22, 2022

Yes, this is about removing unneeded parentheses. Where there's lots of binary operators, we generate quite a lot of these.

roji added a commit to roji/efcore that referenced this issue Jan 22, 2022
Requiring provider opt-in, and falling back to a safer approach if the provider hasn't defined anything

Closes dotnet#27253
@roji roji changed the title Allow providers to specify SQL operator precedence and associativity Remove parentheses by allowing providers to specify SQL operator precedence and associativity Jan 22, 2022
@AndriySvyryd AndriySvyryd added this to the Backlog milestone Feb 15, 2022
@axelgenus
Copy link

It's not an aesthetic issue alone. I am experiencing SQL Server returning "Some part of your SQL statement is nested too deeply" because of this.

I guess I need to use an interceptor to hijack and modify (regex?) the query before is actually sent to SQL Server.

@roji
Copy link
Member Author

roji commented Apr 6, 2022

@axelgenus have you verified that simply removing the parentheses makes the error go away? If so, can you submit a code sample showing that?

@axelgenus
Copy link

@axelgenus have you verified that simply removing the parentheses makes the error go away?

Yes, removing the parentheses makes the error go away.

My test case is quite simple: in one of my projects I use path-like fields to define the ownership of each data row. When a client ask for some data, the web service extract the authorizations from cache and generate a dynamic LINQ lambda expression which, in turn, is applied to EF Core queries. The result SQL statement is something like this:

SELECT [...] WHERE ((((...(Owner LIKE 'keyword1/%' OR (Owner LIKE 'keyword2/%' OR [...] (Owner LIKE 'keywordN/%'))

which can be rewritten as:

SELECT [...] WHERE (Owner LIKE 'keyword1/%' OR Owner LIKE 'keyword2/%' OR [...] Owner LIKE 'keywordN/%')

Of course the count of nested parentheses depends on the assigned authorizations to the specific user. Some users have A LOT of authorizations (> 1k). I defined the tables with a clustered index on the field defining the rows ownership and the LIKE statements make use of the index so the performances are really good. Unfortunately SQL Server supports a limited number of nested parentheses.

I guess that EF Core introduces redundant parentheses mostly because this is the most safe way to handle SQL code generation.

If so, can you submit a code sample showing that?

I'll prepare a test project to reproduce the issue.

@smitpatel
Copy link
Contributor

That is a skewed binary tree causing nesting. It has already been fixed in 7.0

@axelgenus
Copy link

That is a skewed binary tree causing nesting. It has already been fixed in 7.0

Is it possible to fix the behaviour in EFCore 6.0? I was planning on writing a command interceptor to remove redundant parentheses but if there is a "native" way I can make it work on current version of EFCore it would be better.

Can you please point me to the commit with the fix?

@ajcvickers
Copy link
Contributor

Duplicate of #26767

@ajcvickers ajcvickers marked this as a duplicate of #26767 Dec 6, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Dec 6, 2022
@ajcvickers ajcvickers removed this from the Backlog milestone Dec 6, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

6 participants