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 parentheses work #27177

Closed
wants to merge 1 commit into from
Closed

SQL parentheses work #27177

wants to merge 1 commit into from

Conversation

roji
Copy link
Member

@roji roji commented Jan 12, 2022

The 1st commit adds parentheses around IS NULL as long as it's not inside AND/OR - this corresponds to the precedence of all four databases (see below for docs). This seems simple and riskless.

However, I went further in the 2nd commit and generalized this to a more complete "operator information table", where each operator has a numeric precedence and boolean associativity. We now generate parentheses only where precedence requires it, and don't put parentheses if the same associative operator is specified twice (a + b + c + d is fine, a == b == c == d is not (with differing types)). This removes a lot of additional unnecessary parentheses e.g. WHERE (([b].[Foo] * 3) + 2) > 8 becomes WHERE [b].[Foo] * 3 + 2 > 8. Provider extensibility allows changing precedence and adding non-standard operators.

All baselines have been changed so you can see the effects. This change is not without risk; if the new precedence table is somehow incorrect for some database, that could yield incorrect data. I do think we should do it, but add ample testing which I'll do if we're OK with it.

Operator precedence tables:

Fixes #26767

@roji roji requested a review from smitpatel January 12, 2022 15:13
@roji roji changed the title Parenthesize those nulls SQL parentheses work Jan 12, 2022
@roji roji marked this pull request as draft January 12, 2022 15:25
@roji roji force-pushed the ParenthesizeThoseNulls branch 2 times, most recently from 396ec5b to 37ffb53 Compare January 22, 2022 17:51
@roji
Copy link
Member Author

roji commented Jan 22, 2022

@smitpatel I've changed the approach here in a way that you'll hopefully like better (my main goal in life 🤣).

There is no longer an operator precedence table in relational; the default behavior is safe/conservative, and generates parentheses for most binary expressions exactly as before.

However, providers now have the option to provide operator precedence/associativity information. For those operators where that information is provided, it is used to remove parentheses. But the operator table is now a fully internal (and optional) provider concern.

This is still lacking various test coverage and final checks, so not ready for merge - but will wait on a sign-off on the approach before continuing. If you still don't like it, I can bring to a design discussion.

@roji
Copy link
Member Author

roji commented Sep 9, 2022

Decision: we're going to do this for 8.0.

@roji roji force-pushed the ParenthesizeThoseNulls branch from 37ffb53 to cdfe708 Compare December 1, 2022 12:25
Copy link
Member Author

@roji roji left a comment

Choose a reason for hiding this comment

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

I've rebased and improved this PR in some ways.

@maumar as discussed, if you feel like giving me a hand with test coverage here, that would be great (and it's fun too). As we discussed, a dedicated test suite with its own data seems appropriate (e.g. to test operators over all possible boolean values).

Note that we only need to test one level of nesting here; in an outer expression containing an inner one, parentheses are only needed when the precedence of the outer is higher than that of the inner.

Let me know if you have any questions etc!

// TODO: Test associativity (no parentheses on consecutive e.g. add operations)
// TODO: Test non-associativity of arithmetic operators on floating points aren't associative (because of rounding errors)

// TODO: Move operator/precedence related here, e.g. NullSemanticsQueryTestBase.Bool_not_equal_nullable_int_HasValue,
Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice to concentrate all operator precedence tests in this new test suite (they're kind of spread around at the moment). I've identified a couple here, I can do this after you're done with the rest (or if you feel like it...)

ss => ss.Set<Order>().Where(o => -(-o.OrderID) == o.OrderID),
entryCount: 830);

// TODO: Test associativity (no parentheses on consecutive e.g. add operations)
Copy link
Member Author

Choose a reason for hiding this comment

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

There are a few specific cases I'd like to add, but it's probably better to do it after the rest of this test suite is done (for the data etc.)


[ConditionalTheory]
[MemberData(nameof(Get_binary_arithmetic_data))]
public virtual async Task Binary_arithmetic_operators(ExpressionType outer, ExpressionType inner)
Copy link
Member Author

Choose a reason for hiding this comment

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

@maumar here's a theory which takes care of all arithmetic operator combinations (the easy case...). The general idea would be to extend coverage for other operator types as possible.

@roji
Copy link
Member Author

roji commented Mar 9, 2023

Closing in favor of #30371, which adds testing work on top of this.

@roji roji closed this Mar 9, 2023
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.

Remove unneeded parentheses in SQL queries
2 participants