Optimize translation of string.{Starts,Ends}With

[roji]

If the pattern is constant, we now escape it client-side and send a simple LIKE expression - no need for fancy concatenation and an additional LEFT(LEN()) check.

Fixes #14657
Replaces #14820

[roji]

@smitpatel take a look, it's nice to be working in your branch for the first time :)

Following is a dump of questions/reactions as I was working - a lot of it isn't really actionable or anything, just remarks.

• There's no more NullCompensatedExpression, can I just leave it out?
• When exactly should an expression define Condition=true? I mean, any function returning bool can be used in a condition (but I guess it needs = 1 in SqlServer)? Is it possible to simply detect the situations in which an expression requires "= 1" in SqlServer instead of forcing all expressions to declare this? Otherwise, should SqlFunctionExpression have a ctor that doesn't require condition, defaulting to false?
• I noticed that all the translators are now under Pipeline, which isn't under Internal - is this intentional? Do we really want to expose these publicly?
• So most related tests are OK, although it took me a while to realize that TestSqlLoggerFactory.AssertBaseLine() doesn't actually throw on failure :) I'm guessing this is a temporary situation as you're working on the branch...
• Some related tests in FunkyDataQuery are failing because of NotImplementedException, probably because your branch isn't ready for that yet. Should be fine, every worked on master and I can always take care of it later.
• Async tests are failing (on sqlite), but I guess that's also temporary (they're disabled on SqlServer).
• All the old stuff in MS.EFCore.Query.Expressions (e.g. IMethodCallTranslator) is only there during the transition right?
• LikeExpression's constructor now accepts a typeMapping param after escapeChar, which is an optional param. Maybe move typeMapping to be the first param so escapeChar can default to null like before? Also, various methods are broken wrt EscapeChar nullability (e.g. GetHashCode, Equals()). I can submit a PR but I don't know if this is somehow intentional or how widespread the problem is etc.
• All the [CanBeNull]s seem to be missing, this will make it harder to do nullability annotation (when I ever get around to progressing on that...)
• Things are much more verbose now that both the CLR type and type mapping need to be specified all the time :) I wonder if we can't make it a bit lighter, e.g. by passing an expression factory into the translators which knows about the TypeMappingSource and exposes methods similar to the ones found on Expression. So I'd be able to call factory.Equal(x, y), and it would know it returns a bool (because Equal), and would know to look up the type mapping for bool in the type mapping source. This would bring the code back to looking a bit more like regular expression code, at least for stuff like Equal, boolean logic, etc. What do you think? Or have I horribly misunderstood everything?
• Rather than having TypeMappingApplyingExpressionVisitor with type inference logic specific to each expression type, should we consider moving that logic to the expression types themselves? Or are you think that someone may want to override the inference behavior? Do you have a scenario in mind?
• Possibly add an overload of TypeMappingApplyingExpressionVisitor.ApplyTypeMapping() that only accepts an expression, and implicitly applies its type mapping (i.e. internally calls typeMappingSource.FindMapping(expression.Type)?

[smitpatel]

Answers in the same order

• Yes. It will be added when @maumar implements null semantics. It is marker interface for null semantics to not apply null semantics inside of it. I am not sure what form it would take.
• That is weird SqlServer behavior. A comparison does not return "bool", it is just a condition. Currently, if the expression is treated as condition then condition is true set. I can think about making it SqlServer specific but that could be hard. Especially for SqlFunctionExpression which are kind of opaque to EF Core. (we don't try to interprete function. We can add default may be. Let me think a bit more about it.
• One namespace to rule them all. It's temporary, we will do API review to settle namespace once everything passes.
• SQL Assertions has been disabled. I guess I can re-enable them. I had disabled it in earlier days when I was making materialization work and SQL wasn't being printed as pretty as before.
• NotImplementedException is fine. There are few queryable operators not implemented so it will be there till implementation is written.
• Async got no love. While writing new pipeline I disabled async testing (through IsAsyncData) but that does not exclude all async tests. Async still uses old pipeline. But then I removed all member/method call translator so they may fail for some scenarios and pass for some.
• Yes, I am in process of removing Member/method translators.
• I was discussing with @divega yesterday only about having some parameters options. Perhaps, we can move Type & TypeMapping at start so any expression can have optional parameters. We can improve expressions as we see fit. As for Equals/GetHashcode, it should be just in LikeExpression. Most other expressions where I have passed null, I already fixed those bugs.
• I intend to add nullability annotation in one form or another when this would be merged to master. Either jetbrains annotation or nullable types.
• Again, given verbosity of creating each expression, I am already thinking of ExpressionFactory. It could certainly help with comparisons avoiding type mapping etc. and can have injected TypeMapping source.
• Mainly 2 issues in moving to Expression itself. The inference needs to look up type mapping at times. Expressions don't contain services. Provider may want to override SqlFunction inference. That one is we cannot infer any type mapping.
• I thought about that. But I feel it is slippery slope. User may accidently not pass any type mapping without thinking thoroughly if it is needed or not. Also it causes ambiguity in terms of if the type mapping should be default typemapping or pulled from bottom of tree. Currently you can pass null meaning the subtree itself will decide the typemapping or pass default mapping by finding it yourself. Probably best option would be to have a method to call apply default type mapping. Let me think bit more. (then again, finding default in SqlServer.NTS is bit different because you also pass the store Type)

[smitpatel]

Can the pattern be non-string? or this is writing null check in fancy way?

[roji]

Fancy null check :)

[smitpatel]

Invalid comment?
Apart from that, if you are passing constant of string, why would you need char type mapping?

[roji]

Well, I would have preferred to pass a constant of char, but neither SQLServer nor Sqlite support it well (on Sqlite the literal generation of char gives a number...).

[roji]

Thanks for all the answers, makes sense... I'll leave it to you to think about it, and also to tell me whenever you think an area is mature enough for me to get involved.

BTW the build seems to be failing because of an unrelated compilation error