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

DbFunctions compared to NULL are ignored and break the query builder #18547

Closed
rogerfar opened this issue Oct 23, 2019 · 6 comments · Fixed by #18694
Closed

DbFunctions compared to NULL are ignored and break the query builder #18547

rogerfar opened this issue Oct 23, 2019 · 6 comments · Fixed by #18694
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Milestone

Comments

@rogerfar
Copy link

DbFunctions that are comparing to NULL are ignored in the query and even break other (valid) conditions.

For example:

    public class DataContext : DbContext
    {
        private static readonly LoggerFactory LoggerFactory =
            new LoggerFactory(new[]
            {
                new DebugLoggerProvider()
            });

        public DbSet<Course> Courses { get; set; }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            optionsBuilder.UseSqlServer("Data Source=.;Initial Catalog=Test;Integrated Security=True;MultipleActiveResultSets=True;");
            optionsBuilder.UseLoggerFactory(LoggerFactory);
            optionsBuilder.EnableDetailedErrors();
            optionsBuilder.EnableSensitiveDataLogging();
        }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            base.OnModelCreating(modelBuilder);

            modelBuilder.HasDbFunction(typeof(DataContext).GetMethod(nameof(JsonValue)))
                        .HasTranslation(args =>
                        {
                            var arguments = args.ToList();
                            var a = (String) ((SqlConstantExpression) arguments[0]).Value;
                            arguments[0] = new SqlConstantExpression(Expression.Constant(a), RelationalTypeMapping.NullMapping);

                            return SqlFunctionExpression.Create("JSON_VALUE", arguments, typeof(String), null);
                        });
        }

        public static String JsonValue(String column, String path)
        {
            throw new NotSupportedException();
        }
    }

    public class Course
    {
        [Key]
        public Guid CourseId { get; set; }

        public String CourseName { get; set; }
        public String MetaData { get; set; }
    }

    public class MetaData
    {
        public String TestParam { get; set; }
    }

When I run:
context.Courses.FirstOrDefault(m => DataContext.JsonValue("MetaData", "$.TestParam") == "Test 123");
Works as expected, but when I run:
context.Courses.FirstOrDefault(m => DataContext.JsonValue("MetaData", "$.TestParam") == null);
it doesn't work, the generated query is odd as well:

SELECT TOP(1) [c].[CourseId], [c].[CourseName], [c].[MetaData]
FROM [Courses] AS [c]
WHERE CAST(0 AS bit) = CAST(1 AS bit)

When chaining multiple conditions, like this:
context.Courses.FirstOrDefault(m => m.CourseName == "Test 1" && DataContext.JsonValue("MetaData", "$.TestParam") == null);
It gives the same result.

Here is a demo repo:
test-repo.zip

@smitpatel
Copy link
Member

See #11295 (comment) on how to write translation for JSON_VALUE.

Further, don't pass Metadata as string rather use m.Metadata. That will make sure to use exact column rather than trying to guess that query has Metadata column. It will also solve your issue of writing custom translation like that.

@rogerfar
Copy link
Author

rogerfar commented Oct 23, 2019

That seems to work yes, except in our actual application MetaData is a class and uses a converter:

builder.Property(e => e.MetaData)
                   .HasConversion(v => JsonConvert.SerializeObject(v, new JsonSerializerSettings {NullValueHandling = NullValueHandling.Ignore}),
                                  v => JsonConvert.DeserializeObject<MetaData>(v, new JsonSerializerSettings {NullValueHandling = NullValueHandling.Ignore}));

This is why I had to rewrite your suggested solution, it doesn't know what MetaData is and how to deal with that..

Anyway, is there a reason that my solution breaks though? I don't see what causes the issue and breaking the whole query builder.

@smitpatel
Copy link
Member

You declare your JsonValue method in C# to take object of your class and use HasParameter("column").HasStoreType("nvarchar(max)");

The solution breaks due to a bug. We assume that if FUNC(A,B) == null only when either A or B is null. In your case since you passed constant value none of them is null so we simplified it to false. That is one more reason to use actual metadata then string column name via constant expression as by using constant you lose all the metadata EF knows about. (In complex query it will generate invalid SQL too if column gets aliased.)

@rogerfar
Copy link
Author

I tried setting it to Object but then I run into this:

The parameter 'column' for the DbFunction 'DataContext.JsonValue' has an invalid type 'object'. Ensure the parameter type can be mapped by the current provider.'

Also when I set it to the MetaData type it gives the same error. I thought I could solve that with the HasTranslation but it doesn't even get that far.

My solution is just a workaround until full JSON support lands, I'm only using it sparingly where needed, and for now I removed the conditions on the DB level and do them in memory instead.

@smitpatel
Copy link
Member

Hence you need to use this

HasParameter("column").HasStoreType("nvarchar(max)");

@rogerfar
Copy link
Author

Ah OK now I see, that makes sense, works perfectly!

@ajcvickers ajcvickers added this to the 3.1.0 milestone Oct 25, 2019
maumar added a commit that referenced this issue Nov 1, 2019
Resolves #17543 - Queries really slow due to null checks
Resolves #18525 - Query: optimize binary expression AndAlso and OrElse where left and right are the same
Resolves #18547 - DbFunctions compared to NULL are ignored and break the query builder

#17543

Before when rewriting null comparisons we would always remove possibility of nulls in the resulting expression. This is not always needed, specifically in predicates, where it doesn't really matter if expression returns false or null - the result is the same.
Now we detect those cases and apply "simplified" null expansion which should lead to better performance.

#18525

Null semantics initially expands expressions to a verbose form which then gets simplified in query optimizer. One of the optimizations added is when we do AND/OR where both sides are the same. Other small improvements have been added in this change also.

#18525

Previously we assumed that function can only be null if at least one of its arguments is null. This is the case for most built-in functions, but not all. This also goes for user defined functions which can return arbitrary results.
Fix is to treat all functions as potentially nullable. This leads to worse queries is some cases, but is also mitigated by other improvements added along side.
In the future we will provide metadata to better determine given function's nullability.

Also fixed a few small bug found along the way - incorrect Equals comparison for SqlExpression, for cases when TypeMapping was null.
@ajcvickers ajcvickers added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Nov 1, 2019
maumar added a commit that referenced this issue Nov 1, 2019
Resolves #17543 - Queries really slow due to null checks
Resolves #18525 - Query: optimize binary expression AndAlso and OrElse where left and right are the same
Resolves #18547 - DbFunctions compared to NULL are ignored and break the query builder

#17543

Before when rewriting null comparisons we would always remove possibility of nulls in the resulting expression. This is not always needed, specifically in predicates, where it doesn't really matter if expression returns false or null - the result is the same.
Now we detect those cases and apply "simplified" null expansion which should lead to better performance.

#18525

Null semantics initially expands expressions to a verbose form which then gets simplified in query optimizer. One of the optimizations added is when we do AND/OR where both sides are the same. Other small improvements have been added in this change also.

#18525

Previously we assumed that function can only be null if at least one of its arguments is null. This is the case for most built-in functions, but not all. This also goes for user defined functions which can return arbitrary results.
Fix is to treat all functions as potentially nullable. This leads to worse queries is some cases, but is also mitigated by other improvements added along side.
In the future we will provide metadata to better determine given function's nullability.

Also fixed a few small bug found along the way - incorrect Equals comparison for SqlExpression, for cases when TypeMapping was null.

Additional refactoring:
- removed the second pass of sql optimizations (we do it later when sniffing parameter values anyway)
- consolidated NullComparisonTransformingExpressionVisitor into SqlExpressionOptimizingExpressionVisitor
maumar added a commit that referenced this issue Nov 1, 2019
Resolves #17543 - Queries really slow due to null checks
Resolves #18525 - Query: optimize binary expression AndAlso and OrElse where left and right are the same
Resolves #18547 - DbFunctions compared to NULL are ignored and break the query builder

#17543

Before when rewriting null comparisons we would always remove possibility of nulls in the resulting expression. This is not always needed, specifically in predicates, where it doesn't really matter if expression returns false or null - the result is the same.
Now we detect those cases and apply "simplified" null expansion which should lead to better performance.

#18525

Null semantics initially expands expressions to a verbose form which then gets simplified in query optimizer. One of the optimizations added is when we do AND/OR where both sides are the same. Other small improvements have been added in this change also.

#18547

Previously we assumed that function can only be null if at least one of its arguments is null. This is the case for most built-in functions, but not all. This also goes for user defined functions which can return arbitrary results.
Fix is to treat all functions as potentially nullable. This leads to worse queries is some cases, but is also mitigated by other improvements added along side.
In the future we will provide metadata to better determine given function's nullability.

Also fixed a few small bug found along the way - incorrect Equals comparison for SqlExpression, for cases when TypeMapping was null.

Additional refactoring:
- removed the second pass of sql optimizations (we do it later when sniffing parameter values anyway)
- consolidated NullComparisonTransformingExpressionVisitor into SqlExpressionOptimizingExpressionVisitor
@maumar maumar closed this as completed in dd92ba3 Nov 2, 2019
@ajcvickers ajcvickers modified the milestones: 3.1.0-preview3, 3.1.0 Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. customer-reported type-bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants