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

Adding procedural test generation to test interactions between various operators. #30280

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Feb 14, 2023

Testing up to 6 sources (3 pairs for predicate, 2 trios for projection). Entities have 3-4 max rows so even if all 6 are used the result set is not too large. Unused sources are pruned so usually we end up using 3-4 sources.

@maumar maumar requested a review from roji February 14, 2023 08:58
@maumar maumar force-pushed the procedural_tests_go_brrrr branch 2 times, most recently from df94674 to 1823f1b Compare February 14, 2023 09:11
Copy link
Member

@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.

This is... complex 🤣

But 🐑 🇮🇹, most of this is manual testing only - and if it caught some bugs, it's great.


Unaries = new()
{
(typeof(string), typeof(bool), x => Expression.Equal(x, Expression.Constant(null, typeof(string)))),
Copy link
Member

Choose a reason for hiding this comment

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

I think unary equality (as equality to null) is our own thing in EF (right?); in other words, are we expected to actually translate this in a query expression tree given by the user? If it's only an internal temporary representation thing, we probably don't need to be testing for it here?

Copy link
Contributor Author

@maumar maumar Feb 28, 2023

Choose a reason for hiding this comment

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

We use ExpressionType.Equal/ExpressionType.Equal internally in EF to represent null equality, but here there reason is a bit different. Those Unaries/Binaries lists store operations you can perform given the input, and it tells you the type of output. So here we have input being string (however we arrived at that input type), we know we can apply == null operation and we get bool back. So, say, we have two source entities (a, b) that have string values, as first step we may have generated string concat operation: a.Value + b.Value (two string inputs, and string output), and then in second step, we have string input (i.e. the result of concat), and we can apply the == null. So the end result would be

from a in ctx.Set<OperatorEntityString>
from b in ctx.Set<OperatorEntityString>
select (a.Value + b.Value) == null

which is somewhat interesting from parens perspective:

(a.Value + b.Value) IS NULL vs a.Value + (b.Value IS NULL)

In other words, in this list just look at the lambda, which is the actual expression that will get generated (and is a normal binary), but stored it in Unary, because it only really has one variable argument, the second one being null. Same goes for arg LIKE 'A%' - technically LIke is a binary operation, but the second argument is fixed so we store as unary here

// we also force nesting if we end up with an expected node that we don't have the root entity for
// this can happen when we use convert - e.g. we only have int sources, but we expect long
var rollAddDepth = random.Next(maxDepth);
if (rollAddDepth >= currentDepth)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I'm understanding this correctly, but this seems to nest multiple expressions up to this depth... If so, I think that for precedence issues it's always enough to check just two expressions (one nested within the other) - is that a possible simplification here? Or am I totally misunderstanding?


return chosenExpresion.Expression;
}
else
Copy link
Member

Choose a reason for hiding this comment

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

can remove (also in other places in this file)

@roji
Copy link
Member

roji commented Feb 23, 2023

BTW we have various existing tests which check parentheses stuff, these can in theory be moved here at some point, so we concentrate all the operator/parentheses tests in one place. Not necessarily worth spending time on now, but it's good to have the this one place now.

@roji
Copy link
Member

roji commented Feb 23, 2023

One last thought - because of the complexity of the procedural testing, it may be worth splitting the (complex) procedural testing to something like OperatorsProceduralQueryTestBase, so that we have the non-procedural regression tests (Regression_test1, 2...) in one easy, clear place.

…s operators.

Testing up to 6 sources (3 pairs for predicate, 2 trios for projection).
Entities have 3-4 max rows so even if all 6 are used the result set is not too large.
Unused sources are pruned so usually we end up using 3-4 sources.
@maumar maumar merged commit b4ccad2 into main Feb 28, 2023
@maumar maumar deleted the procedural_tests_go_brrrr branch February 28, 2023 05:41
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.

2 participants