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

SQL builder appends '= 1' after custom sql function regardless return type #11316

Closed
elepner opened this issue Mar 17, 2018 · 4 comments
Closed

Comments

@elepner
Copy link

elepner commented Mar 17, 2018

I'm digging into EF Core internals. I try to extend EF capabilities by adding CONTAINS function support from SQL Server text analysis package.
What I have done:
Extended SqlServerCompositeMethodCallTranslator where I append custom FreeTextTranslator
This FreeTextTranslator translates custom extension method ContainsText of string to CONTAINS query.

Here's the code of translator (idea I borrowed from SqlServerContainsOptimizedTranslator):

    public class FreeTextTranslator : IMethodCallTranslator
    {

        private static readonly MethodInfo _methodInfo
            = typeof(StringExt).GetRuntimeMethod(nameof(StringExt.ContainsText), new[] {typeof(string), typeof(string)});

        public Expression Translate(MethodCallExpression methodCallExpression)
        {
            
            if (methodCallExpression.Method != _methodInfo) return null;

            var patternExpression = methodCallExpression.Arguments[1];
            var objectExpression = (ColumnExpression) methodCallExpression.Arguments[0];

            var patternConstant = patternExpression as ConstantExpression;
            if(patternConstant == null) throw new NotSupportedException("Dynamic variable execution is not supported so far");
            var sqlExpression =
                new SqlFunctionExpression("CONTAINS", typeof(bool),
                    new[] { objectExpression, patternExpression });
            
            //var sqlExpression = new SqlFragmentExpression($"CONTAINS({objectExpression.Name}, N'{patternConstant.Value}')");

            return sqlExpression;
        }
    }

This code almost works, however this code:

var query = dbContext.Posts.Where(x => x.Content.ContainsText("egg")).ToArray();

translates to that SQL with =1 in the end which is not syntactically correct

SELECT [x].[Id], [x].[AuthorId], [x].[BlogId], [x].[Content], [x].[Created], [x].[Rating], [x].[Title]
      FROM [Posts] AS [x]
      WHERE CONTAINS([x].[Content], N'egg') = 1

I tried to use SqlFragmentExpression but it didn't help either. Here you can find full code.

I think SQL builder should respect SqlFunction return type and not append = 1 by default.

Further technical details

EF Core version: 2.1.0-preview1-final
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows 10 16299
IDE: Visual Studio 2017

@pmiddleton
Copy link
Contributor

SqlFunctionExpressions must have a return type, in your example it is bool. In your case a default compare to 1 is being added to the expression tree.

You would need to update the query generator to ignore the comparison when generating the sql for your function.

Freetext support for Sql Server was added in pr #10480. Take a look at the changes made to SqlServerQuerySqlGenerator.cs to see what I am talking about.

@elepner
Copy link
Author

elepner commented Mar 18, 2018

Ok, I see that there's a change in VisitBinary method.

Don't you think that that if statement in this pr is quite weird and should be more generic? It will fall back to the base method if function name is not "FREETEXT" (e.g. "CONTAINS"). It should respect return type I guess.

@smitpatel
Copy link
Member

Duplicate of #9143

The SQL Gen phase has a expression visitor which converts values to condition and vice-versa based on part of SQL statement. That visitor is private at present so provider cannot override that code. Presently that visitor assumes that all SqlFunctionExpression are of value type. Hence when it appears in where predicate it will convert to condition so append = 1. That VisitBinary method right now is the work-around that specific issue in SqlServer provider.

Till #9143 is fixed the only way to fix this is to provider Custom Sql Gen which does the same as FREETEXT.

@elepner
Copy link
Author

elepner commented Mar 19, 2018

Thanks for quick reply :) Now the problem is 100% clear. I wish I could fix it by myself and contribute your great project, but I'm not smart enough for that :)

@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants