SQL translation should be more generic and operate on end results

[mqudsi]

Following up on the discussion in #12517, I believe there's a fundamental issue with the way SQL translation is currently handled. If there is an expression that cannot be directly mapped to an SQL equivalent, but that expression evaluates to a data type that can be directly plugged in to an SQL query, then that expression should be evaluated and its result used instead of the SQL translation giving up and the query being evaluated locally rather than on the database server.

To illustrate with a concrete example:

    public class Database : DbContext
    {
        ....
        public IQueryable<Listing> ActiveListings => Listings
            .Where(x => x.Status != ListingStatus.Sold)
            .OrderByDescending(x => x.RenewTime)
            .Where(x => x.RenewTime > DateTime.UtcNow.AddDays(-7));
    }

In the latest published EFCore w/ SQLite provider releases, this cannot be evaluated in the database as AddDays(xx) does not have a translation implemented.

However, simply doing the following makes SQL translation work and the query is fully evaluated on the DB server rather than the ASP.NET Core host:

    public class Database : DbContext
    {
        ....
        DateTime ExpiryCompare => DateTime.UtcNow.AddDays(-7);
        public IQueryable<Listing> ActiveListings => Listings
            .Where(x => x.Status != ListingStatus.Sold)
            .OrderByDescending(x => x.RenewTime)
            .Where(x => x.RenewTime > ExpiryCompare);
    }

More generally, if the translation of a given subexpression does not have a translation expressly provided by the provider, and that subexpression does not depend on the value of the record being evaluated (i.e. for a lambda x => x == Foo(..), Foo does not utilize the value of x) and the return type of Foo(...) can be used to directly evaluate the expression on the database server, then that subexpression should be evaluated and its result plugged in before translation is attempted once more.

[tuespetre]

I agree. I think a ‘second pass’ at untranslatable expressions that can extract client-side evaluatable expressions into parameters is the way to go. I have mentioned this elsewhere before but I am not sure what the team thinks of it.

[jzabroski]

This should not be a second pass, but rather a first pass, before calling EF.

This can be done outside EF, or on top of EF. Please see the excellent expression transformation library, erecruit.Expr.

This library is agnostic to EF, although there are some clever optimizations that handle problems with how EF caches queries with IEnumerable.Contains expressions.

The library takes a divide-and-conquer approach, by implementing a few basic abstractions:

• Common sub-expression substitution via Expression.ReplaceParameter and Subst.Expr
  ** e.g., suppose you have a interface ICodeName { string Code { get; set; } string Name { get; set; } }, then you can Subst.Expr(objectInheritingICodeName, (ICodeName codeName) => new CodeName { Code = codeName.Code, Name = codeName.Name }); and voila, you have created a re-usable way to extract common fields from the database in your query projection
• Boolean combinators, such as And and Or
• List comprehensions such as Fold
• Function composition via Compose
• Syntactic sugar for building expression trees via Expr.Create: var trueStatement = Expr.Create(() => true) will generate a boolean expression that returns true
• Expression expansion via Expand

If you're not familiar with this library, it is fundamentally superior to Joseph Albahari's PredicateBuilder. Note that erecruit.Expr does not leak to the caller, while PredicateBuilder does: With PredicateBuilder, you must call AsExpandable() first, meaning the API designer must know ahead of time you want to use PredicateBuilder. Instead, erecruit.Expr lets you just call Expand() at the very end of your query chain. Also, the only penalty for calling Expand multiple times is performance.

The only gotcha with using erecruit.Expr is it has a max recursion depth equal to the available call stack of the underlying .NET host platform.

There are various reasons why doing this request internally is a mistake:

• Handling of sargability
• Harder to see developer's intentions.
• Crazier stack traces.

Please close.

[smitpatel]

Duplicate of #12338

[smitpatel]

I would recommend reading this comment and later discussion.

[tuespetre]

@jzabroski I do not mean a ‘second pass’ as in a pass over the entire expression tree. When EF hits a a selector/predicate/etc. it tries to rewrite certain expressions into SQL-translatable expressions. If it encounters, for instance, a member access or method call expression it cannot rewrite, it tends to return null and the translation of that subtree is aborted. What I am saying is that at that point, there is a ‘second chance’ at extracting that portion of the subtree into a parameterized expression rather than aborting translation. This is not specific to the DateTime/SQLite issue; it is a general solution to a general problem and the ‘base’ SQL query provider (EF Core’s relational provider in this case) is the most suitable place for such a mechanism.

[jzabroski]

@tuespetre There's a couple reasons you're incorrect or coming up with the wrong solution.

1. Your proposal will add overhead, as you will not be able to cache your query until you do the EF pass! erecruit.Expr caches in front of EF the hoisting mechanism you want. This means EF can just hash the expression tree, without computing anything.
2. Preserving the semantic meaning of queries is also a concern, as @smitpatel points out in his comment
3. Why not write a Roslyn code analyzer for EF to detect "If it encounters, for instance, a member access or method call expression it cannot rewrite, it tends to return null and the translation of that subtree is aborted."

A Roslyn code analyzer for EF could also potentially do sarg-ability analysis. I believe there are many ways people have been tripped up with the sarg-ability of LINQ queries in my career:

1. Linq 2 Sql handled CHAR(n) data type comparisons poorly
   from t in dbContext.Trades where t.Side == 'B' select t;
   would generate as:
   SELECT [t0].Quantity, [t0].Price, [t0].Side FROM [dbo].[Trade] AS [t0] WHERE UNICODE([t0].[Side]) = @p1
2. string.Equals behaving differently from == operator
3. Converting enums to strings and comparing the value to another string.

Then there is just "API Coverage" concerns:

1. Linq 2 Sql didn't support many primitive string methods:
   from t in dbContext.Persons where string.IsNullOrWhitespace(t.Name) select t; /* RUN-TIME ERROR! */

[tuespetre]

@jzabroski it does not prevent caching. The subtree is pulled out into a lambda expression that receives the correct closure instance as a parameter at runtime, when it is executed to produce the value for that execution of the query. I have implemented it, tested it, and used it in production. It’s good 👍

[mqudsi]

At the risk of commenting on a decision that's been sealed beyond reconsideration, I feel like there is room for a healthy discussion here out of which some good can come.

Sargability is extremely important to me and I've filed defect reports with several open source database engines and products to offer some (too-clever) workarounds to get certain datetime operations to use the index, so I am not blithely making suggestions that would break sargability here.

As someone that has just begun using EFCore and did not use EF previously, my "fresh take" on it was that the default would be for all efforts to be focused on getting my C# LINQ expression translated to code that could most optimally run on the db server. The existence of the EF.Functions extension class implied to me that to take advantage of certain db-level optimizations (such as maintaining sargability of lookups) I should use these functions from the db provider implementation to get optimal results (instead of doing the equivalent DateTime.xxx or string.xxx in C#).

Anyone that's handwritten queries knows about optimizing the indexes and queries together to come up with fast, efficient queries and is familiar with the benefits of limiting the amount of rows carried back from the db server, etc. But what EFCore has done here is take those common concerns and give them the backseat to an even bigger problem we have to worry about now: whether or not the code will even be executed on the db in the first place. The benefits of C#-to-SQL translation are enormous and it's an extremely laudable effort, but I feel like it should be done in a way where precedence is given to in-db execution instead of to maximized lambda support at the cost of runtime warnings masking massive performance issues for cases that could be trivially supported as a matter of design decision.

All that aside and with the caveat that I don't know how the code that does this currently looks like, I don't understand why SQL translation can't consider the return type of a function call not dependent on the value of a record column in its decision to execute-then-translate or translate-then-execute flow.

For the case where the lambda invokes a C# function on the column/output of the database, it's a clearcut case of "the user wants to invoke local code to evaluate db values, without writing a for loop" which then proceeds to answer the question "is there an optimization we can perform to eliminate the need to bring this data back to the application server for local evaluation" in which case a db construct can be used instead of the local C# function call and the translation would be over. In case the answer is no, it's a shortcut to "unfortunately this needs to be evaluated locally" and again the translation would be over.

But in the case that the db column/value is not an input to a subexpression in the lambda, the same steps can be applied only there's a recourse instead of giving up. You start with the same question, "is there a db-level construct that can optimize the entire expression" only when the result is no, the question becomes "can we reduce the c# expression into a different one that we could perhaps optimize?" which can be iteratively evaluated until that answer becomes "no."

As concrete examples,

• .Where(x => x.Day == foo.Day) has a hardcoded rule for DateTime.Day == DateTime.Day that can be applied and the translation generates a sargable expression to be evaluated on the db server
• .Where(x => Foo(x)) and EFCore has no knowledge of Foo(), this must be evaluated locally, early abort of translation
• .Where(x => x == Foo() where Foo() is something EFCore knows about, for instance, DateTime.UtcNow.ToLocalTime() and a query to do this in the db can be used
• .Where(x => x == Foo() where Foo() is not something EFCore knows about but with Foo() evaluating to a DateTime EFCore should be able to reduce that to an expression it can translate
• .Where(x => x.Day == Foo().Day exhibits the best of both worlds, where a previously unsupported LINQ expression can be evaluated a step at a time, until it arrives at <DateTime>.Day which is supported yielding the sargable <DateTime>.Day == <DateTime>.Day expression, which can ultimately be evaluated in the db for maximum performance and use the index to boot.

[divega]

Reopening because I agree there are some points in this thread that can use more discussion. I will catch up with @smitpatel next week to make sure I understand his point of view.

[smitpatel]

@mqudsi - Explain in detail the last case .Where(x => x.Day == Foo().Day) would be translated to in SQL and why it is sargable compared to what EF Core currently generates for it.