Query: Enable translations that generate different SQL depending on a value to be cached

[MikeDempseyFL]

The current implementation of the relational StringCompareTranslator only handles the String.Compare(s1, s2) method.
On our database that would be indeterminate since the database session may be set to use case specific, or not specific, comparisons.
I want to force this method to always use case specific comparisons and to also support the String.Compare(s1, s2, ignoreCase) method in our EF provider.

In order to do this it appears that I need to add our own StringCompareTranslator and also add an override for the VisitStringCompare method in our QuerySqlGenerator.

My current design does work for 3 of the 5 possible scenarios but the remaining 2 [less common] cases would require some other way to pass information from the translator to the visitor.

I am currently adding the required text decoration in the translator and using it in the visitor, but I can only decorate constant expressions and simple column references.
If one term is a more complex column reference [e.g. e.name.Substring(1,3) rather than just e.name.] and the other term is either a variable [i.e. a parameter] or another complex column reference then I can't add the required information.

Note that in order to support simple column references I essentially have to 'visit' the column reference in the translator in order to turn it into a constant expression [e.g. "e"."name" (CS)] which kind of bends the rules already.

An example of a case specific comparison between a column reference and a variable would be:
"e"."name" = ? (CS)

Can anyone suggest an alternative way to handle this without jumping through hoops to only partially support the requirement.

Further technical details

EF Core version: 2.1
Database Provider: Teradata
Operating system: Windows 10
IDE: Visual Studio 2017 15.6

[smitpatel]

@MikeDempseyFL - We will need more information on this to effectively understand the issue. You are saying that your design works for 3 out of 5 scenarios. Can you elaborate those scenarios? Further can you also describe what information are you trying to pass on and why you need to look at the expressions specifically?

[MikeDempseyFL]

Currently, in the translator, if the term is either a constant or a simple column reference we replace it with a new [string] constant expression that includes all required quotes and the suffix.
Example for a case specific comparison:

Constant abc becomes 'abc' (CS)
Column Ref. [e].Name becomes "e"."Name" (CS)

In VisitStringCompare, if the term is [now] a constant we simply append the value to the SQL rather than calling a visitor. (The visitors would have added the quotes in the wrong place.)
We also extract the term in parens at the end of the constant value.

If a term is a parameter, or a more complex expression, we call its visitor as usual, but then append the extracted term (in parens) if we have one.

So as long as at least one of the terms is either a constant or a simple column reference we will successfully generate the SQL with the required '(CS)' or '(Not CS)' suffixes on both sides of the comparison operator.

The issue is that if BOTH terms are complex references, or one is a complex reference and the other is a parameter, we have no way to know what suffix to add. This would require somehow passing the value of the ignoreCase switch to the VisitStringCompare() method.
In fact if we could pass that switch we would not need to replace the expressions in the translator. We could do everything in VisitStringCompare() and greatly simplify things.

[smitpatel]

I would suggest not doing any processing based on type of Expression because it complicates thing and generally not a good idea since it breaks the ability to evaluate each part of Expression Tree independently.
Adding additional property in StringCompareExpression could work though since it is not supported in EF Core, its design would be pending.

In the meantime, I suggest getting rid of StringCompareExpression totally at provider level. You can add your custom CaseSensitiveStringCompareExpression which contains all the information you need. You can create it from your method call translator and visit it in QueryGen.
This query will give you an idea what are all places which you may need to update in relevant provider level services.
https://github.com/aspnet/EntityFrameworkCore/search?q=StringCompareExpression&unscoped_q=StringCompareExpression

[MikeDempseyFL]

Thanks.
I was already looking into the idea of creating our own 'custom' expression to use instead of ParameterExpression in some cases. Doing something similar for the StringCompareExpression would probably be the best approach.

[MikeDempseyFL]

I derived my own Expression from StringCompareExpression adding one extra field for the IgnoreCase switch.
That works well as long as the boolean switch is a literal value but if it was a variable then it will be a parameter expression and I don't see any way to get the parameter value from within my VisitStringCompare() method.
Even though LINQ considers it to be a parameter it MUST be processed on the client.

Is it possible to get the value of a parameter from within the Visit methods or is that value not assigned until a later stage? (I saw references to command reuse which would imply that no value is assigned yet.)

[smitpatel]

Which VisitStringCompare method? What is the visitor on question where you are looking for the value? parameter values are only available during QuerySqlGenerator.

[MikeDempseyFL]

The VisitStringCompare method is our own method in our TeradataQuerySqlGenerator which is derived from DefaultQuerySqlGenerator.

[smitpatel]

You have parameter values available there
https://github.com/aspnet/EntityFrameworkCore/blob/2d1767e767a8446d3ed5ae6adbc9c55d6c7823d2/src/EFCore.Relational/Query/Sql/DefaultQuerySqlGenerator.cs#L118

[MikeDempseyFL]

Of course!
I had been looking for a property or method on the ParameterExpression itself. I didn't think to look into the SqlGenerator itself.

The only issue I have now is that since LINQ thinks this ParameterExpression represents a parameter passed to the database - rather than a value that will be used in the visitor to generate the SQL - link will not call the visitor again if this is the only thing that changes before another call.
It simply reuses the previous command thinking that it is passing the new value to the database.
A performance improvement that becomes an issue if the 'parameter' is being consumed on the client - not the database. (as a general rule that probably wont happen often)

[smitpatel]

@MikeDempseyFL - Can your method on server take parameter as arg (or whatever represents the casing)? If not then perhaps the best place to look for is to avoid parametrization. For user defined methods, we use NotParametrizedAttribute. Since this would be method from bcl, you should look into ParameterExtractingExpressionVisitor and try to make sure that when you are visiting 3rd arg of String.Compare method, instead of making a parameter out of it, it extracts value out of it and adds the constant inside ExpressionTree. Then you wouldn't have to worry about parameter Expression anymore.

[MikeDempseyFL]

Thanks. I'll look into the ParameterExtractingExpressionVisitor.

[MikeDempseyFL]

On the surface it looks like I should just derive my own type from ParameterExtractingExpressionVisitor and override the VisitMethodCall() method as follows:

protected override Expression VisitMethodCall(MethodCallExpression methodCallExpression)
        {
            MethodInfo methodInfo = methodCallExpression.Method;
            var args = methodCallExpression.Arguments;

            if (methodInfo.Name == "String.Compare" && args.Count == 3 && args[2] is ParameterExpression paramExpr)
            {
                if (_parameterValues[paramExpr.Name].GetType() == typeof(Boolean))
                {
                    var parameterValue = _parameterValues.RemoveParameter(paramExpr.Name);
                    Expression[] newArgs = { args[0], args[1], Expression.Constant(parameterValue) };
                    return methodCallExpression.Update(methodCallExpression.Object, newArgs);
                }
            }

            return base.VisitMethodCall(methodCallExpression);
        }

Unfortunately all the fields in this type are private and there are no properties defined on them.
The only one I need is _parameterValues but I can't access it.

Also, I didn't see an interface that appears to be intended for this type in order to hook my derived type into the service collection.

[smitpatel]

Another way to solve it would be to set IsCachable in your SqlGenerator. But that property is also private set only. So perhaps there is no way to fix it without changing EF Core unless you want to copy over whole code of ParameterExtractor or SqlGen.
We will discuss this in more detail but so far the pipeline has been if you can put variable in linq ExpressionTree then you should be able to replicate it in SQL tree.

[ajcvickers]

Triage: we should make IsCachable protected, but then also consider this case as part of the overall query work for 3.0, so adding a reference to #12795.

[divega]

@smitpatel let’s either keep this open or create a new issue to try to provide a solution that doesn’t disable caching in the longer term.

[ajcvickers]

Moving this to 3.0 and linking to #12795 to consider as part of Query3.

[divega]

@MikeDempseyFL, @smitpatel made IsCachable protected in #12857. I just want to make sure that you were able to understand his suggestion and that you have been able to address this scenario in your provider.

We would like to re-purpose this issue to track the possibility of handling this kind of scenario in a way that doesn't disable the ability to cache the SQL translation.

[MikeDempseyFL]

Yes I think I understand the solution. The IsCachable property is currently initialized to true in GenerateSql() and then the SelectExpression is visited. So I would set IsCachable to false in my VisitTeradataStringCompare() method. This method is in my class that derives from DefaultQuerySqlGenerator.

[ajcvickers]

@smitpatel to follow up with new issue on parameterization.

[smitpatel]

Filed #14507

Trying to update second level cache to account for parameter value too (rather than just nullability), would bring in a lot of complexity along with inconsistency when to consider value and when not to. Rather than playing with second level cache, we should allow providers to control what can be parameterized. While it is true that SQL is only different and may not need a different compilation but at least for now we should try to keep second level cache only related to null semantics on parameters