#10955 - Using queries within a checked context

[svengeance]

Fixes #10955

This should be fairly straightforward. I followed advice from @smitpatel on the Issue and created a visitor in the query compilation preprocessor to convert all checked expressions to their unchecked equivalent.

I could use some advice on making the tests more thorough. Given that the intention is to make checked/unchecked translations equivalent (minus client evaluation), it'd be nice to evaluate the AssertQuery in both a checked and unchecked context, but I didn't find a pattern for that kind of thing within the GearsOfWar test suite.

As an aside, I tested this implementation by switching all projects to use assembly-wide overflow checking. The only resultant failures were due to client-side overflow evaluation (i.e. passing -1 to a ulong), but all queries passed otherwise.

Cheers

[bricelam]

Part of #14572 (VB turns checking on by default)

[maumar]

Overall problem with this approach is that we strip out all convert nodes. Instead, (per EF Team design decision mentioned by @ajcvickers ) we should only be ignoring convert nodes that are not client-evaluated. So, this code should be part of translation (RelationalQueryableMethodTranslatingExpressionVisitor). The errors you saw when switching all tests to checked were likely from the parameter extraction, which happens at the very beginning, but it's not the only client evaluation that we support. Example of a query that illustrates the problem:

        [ConditionalTheory]
        [MemberData(nameof(IsAsyncData))]
        public virtual Task Checked_context_with_addition_does_not_fail_client_eval(bool isAsync)
        {
            checked
            {
                return AssertQueryScalar(
                    isAsync,
                    ss => ss.Set<LocustLeader>().Select(w => w.ThreatLevel >= ((int)(long?)5 + (long?)ClientMethod(w.ThreatLevel))));
            }
        }

        public static short ClientMethod(short arg) => arg;

With the current approach, this produces the following query plan:

(ShapedQueryExpression: 
    QueryExpression: 
        (Projection Mapping:
        SELECT ((CAST((l.ThreatLevel)) AS bigint))), ((CAST(5 AS bigint))), ((l.ThreatLevel))
        FROM (LocustLeaders AS l)
        WHERE ((l.Discriminator) IN (N'LocustLeader', N'LocustCommander')))
    ShaperExpression: (ProjectionBindingExpression: 0) >= (ProjectionBindingExpression: 1) + (Nullable<long>)(long)GearsOfWarQueryTestBase<GearsOfWarQuerySqlServerFixture>.ClientMethod((ProjectionBindingExpression: 2))
)

Convert nodes around ClientMethod should be checked

[maumar]

wrt testing approach, I think using checked keyword is good enough. The mechanism is niche enough that I don't think we need test infra for it specifically. Please add some tests for client eval scenarios in the projection (similar to one I provided above) and we should be good enough, test-wise.

[svengeance]

@maumar re: testing for client-eval, what would be the best way test specifically that in a client eval scenario we don't remove the checked nodes?

I will transition this back to RelationalTranslator - I had this originally (by simply treating checked nodes as their unchecked equivalent) but decided to pull it out. I wasn't (and still am not to be honest) where the boundary is between the SQL and non-SQL translation elements.

[maumar]

@svengeance only place we allow client eval is projection, so you can create a client method, call it in the Select, make sure that the method takes some column value as argument, so that it doesn't get funletized out. The result of the function can be whatever, so you can just return a constant that would overflow when convert (or other relevant operation) is applied on it. You can then make sure that correct exception is thrown by using Assert.ThrowsAsync

[svengeance]

I've moved the IgnoreChecked visitor to the relational level and am observing the desired effects.

To be frank I still haven't fully grasped the pipeline for translation yet and this was a..hopefully intelligent shot in the dark for placement. My thoughts are that the method I used (RemapLambdaBody) is necessarily called by every server-side evaluation predicate, and that this was the best place to remap all checked nodes to their unchecked variants.

Appreciate your guidance/patience and followup.

[maumar]

@svengeance changes look good! Please squash-rebase on current master and it should be ready to commit.

[smitpatel]

Also update Cosmos & InMemory providers

[svengeance]

Also update Cosmos & InMemory providers

Given that the InMem provider is usually true to C#'s standards as opposed to relational standards, is this checked->unchecked behavior desired there? Or did you mean something different?

I will update Cosmos and add the appropriate test coverage.

[svengeance]

With adding tests to cosmos - from what I'm gathering reading other issues regarding testing this provider it seems you run a cosmos emulator? Is there any documentation on how to set this up and working with EF here so I can write the same few tests, or should I just write a bit blindly and let the CI tackle it? 😄

[smitpatel]

@svengeance - Hold off for InMemory, I will ask team in the meeting on Friday to decide where to go.
For Cosmos testing, just install emulator and start it, tests locally should start running automatically.

[svengeance]

I got the emulator functioning, however I'm having difficulties with proving out functionality. Maybe related to #17246? Issue aside, I can't find any code supporting server-side conversion on VisitUnary like RelationalSqlTranslatingExpressionVisitor has with referencing typemaps and the appropriate CONVERT sql function. To be frank I have little understanding of how Cosmos works, so maybe the logic is there and I'm missing something.
Regular, unchecked conversion (short -> byte) also fails, for what it's worth.

I have the code placed in Cosmos mirroring sql server. Given that conversion seems unsupported yet (or again, I could be missing where?), how should I proceed?

[smitpatel]

@svengeance - For in memory we won't do anything special. If it throws on server side, let it be.

Cosmos does not have server side convert so Convert client eval or throws. ConvertChecked would follow the same. Binary operators would follow similarity with relational.

[svengeance]

Added the cosmos test to verify checked binary is unchecked, I managed to figure out a use-case for the VisitUnary where ConvertChecked is applicable (ignoring casts to nullable types that are the same type in a checked context).

I believe everything should be completed now, if I'm not concerned about in-memory operations here. I've went ahead and rebased as well.

[smitpatel]

Merged to master in #19862
@svengeance - Thank you for contribution!

[svengeance]

Definitely thank you for the guidance on this one, and the introduction to cosmos.