Add DATALENGTH function to SqlServer

[Marusyk]

Add DATALENGTH function to SqlServer

Fixes #19988

Please review,
Thank you in advance

[smitpatel]

Also rebase on latest master so that checks can work. Sorry, we had a build break when you checked out.

[smitpatel]

@Marusyk - Can you also add tests for other data types for which we are adding overloads?

[Marusyk]

Actually, I've tried something like

bool? value = true;
var count = context.Orders
             .Count(c => c.OrderID < EF.Functions.DataLength(value));

but got

InvalidOperationException: The 'DataLength' method is not supported because the query has switched to client-evaluation. Inspect the log to determine which query expressions are triggering client-evaluation.

could you please suggest?

[smitpatel]

Don't use it on local variable. It will cause parameter extraction to run. Since EF.Functions.DataLength(value) does not correlate with range variable c, it will try to evaluate that before running the query hence the error.

[Marusyk]

@smitpatel what should I use then? There is no all types in c to cover all cases

[smitpatel]

Use different entity type. Customer has string, Order has int/date (mapped as datetime). GearOfWar would have DateTimeOffset/TimeSpan/Byte array, fixed/variable size.

[Marusyk]

Unfortunately, I can't add more tests for other data types. Because I don't unserstand why this code works fine:

var count = context.Orders
    .Count(c => 100 < EF.Functions.DataLength(c.OrderDate));

but this one fail

var count = context.Orders
     .Count(c => 100 < EF.Functions.DataLength(c.Freight));

The LINQ expression 'DbSet
.Count(o => (Nullable)o.OrderID < __Functions_0
.DataLength(o.Freight))' could not be translated. Either rewrite the query in a form that can be translated, or switch to client evaluation explicitly by inserting a call to either AsEnumerable(), AsAsyncEnumerable(), ToList(), or ToListAsync(). See https://go.microsoft.com/fwlink/?linkid=2101038 for more information.

Also I tried Employees, something like

var count = context.Employees
     .Count(c => 100 < EF.Functions.DataLength(c.Photo));

the same error.

GearOfWar would have DateTimeOffset/TimeSpan/Byte array, fixed/variable size.

@smitpatel, there is no any GearOfWar in solution!

[smitpatel]

Those properties are ignored in EF Core model.
https://github.com/dotnet/efcore/blob/master/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs

[Marusyk]

As you can see, I added tests to EFCore.SqlServer.FunctionalTests but not to EFCore.Specification.Tests project. Because here are tests for other functions

[smitpatel]

This file will tell you what all is ignored in northwind. https://github.com/dotnet/efcore/blob/master/test/EFCore.Specification.Tests/TestModels/Northwind/NorthwindContext.cs

[Marusyk]

There are only

• bool
• string
• decimal
• DateTime

[smitpatel]

@maumar to rescue. Let him know which other data types you need.
You can add additional tests in GearsOfWarQuerySqlServerTest file too.

[Marusyk]

Should I really add some tests to EFCore.Specification.Tests and others to EFCore.SqlServer.FunctionalTests? 🤨

@maumar please advise

[smitpatel]

@Marusyk - Please add tests to GearsOfWarQuerySqlServerTest file. It is ok to add tests there which are provider specific. The test will run the query an assert the SQL too. https://github.com/dotnet/efcore/blob/master/test/EFCore.Specification.Tests/TestModels/GearsOfWarModel/ shows all the classes mapped in GearsOfWarModel. You can use any entity type as query root and property on them to use DataLength on. Almost all of the properties are mapped, so you would not run into issue like northwind.

Let me know if you are unable to find entity type with a specific type. I can point you to specific property to use for the test.

[Marusyk]

@smitpatel,
DataLength is SQL Server function. Are you sure that we need to add it to EFCore.Specification?

Seems the EFCore.Specification.Tests project doesn't have reference to EFCore.SqlServer project where the DataLength is implemented!

[smitpatel]

You don't add it to EFCore.Specifications.Tests project. You add it to GearsOfWarQuerySqlServerTest class which lives in EFCore.SqlServer.FunctionalTests project which references EFCore.SqlServer project.

You will utilize a model & context defined in EFCore.Specification.Tests project, which is available in EFCore.SqlServer.FunctionalTests project with SqlServer provider configured and you will write queries there.

[Marusyk]

Ok, but you suggested EFCore.Specifications.Tests before

Those properties are ignored in EF Core model.
https://github.com/dotnet/efcore/blob/master/test/EFCore.Specification.Tests/Query/GearsOfWarQueryTestBase.cs

[smitpatel]

Sorry for confusion there.

[Marusyk]

@smitpatel sorry, but I think you need to ckeck the source code.

Did you see how implemented tests in GearsOfWarQuerySqlServerTest ? All tests call method from base class which is GearsOfWarQueryTestBase in EFCore.Specifications.Tests project! But as I said before

DataLength is SQL Server function. Are you sure that we need to add it to EFCore.Specification?

You confusing me and waste my time. We spent a lot of time discussing the simple thing. Can anyone else review?

[AndriySvyryd]

Did you see how implemented tests in GearsOfWarQuerySqlServerTest ? All tests call method from base class which is GearsOfWarQueryTestBase in EFCore.Specifications.Tests project!

Yes, currently all the tests there are overrides of the base ones, but they don't need to be. You can define a new test at that level.

[Marusyk]

Thanks. I'm trying to follow some conventions, and when I see a file with 7500+ line of similar code, then I need some approval. Before it was confusing, because first we were talking about some GearOfWar (which actually doesn't exisit), then GearsOfWarQueryTestBase.cs, and it finally turned out to be GearsOfWarQuerySqlServerTest.cs

I've tried this

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public async Task Where_TimeSpan_Milliseconds2(bool async)
{
    await AssertQuery(
        async,
        ss => ss.Set<Mission>().Where(g => g.Id > EF.Functions.DataLength(g.CodeName)));
}

got again

InvalidOperationException: The 'DataLength' method is not supported because the query has switched to client-evaluation. Inspect the log to determine which query expressions are triggering client-evaluation.

Could you please help and suggest what I'm doing wrong?

[maumar]

@Marusyk The problem you are seeing now happens because test infrastructure tries to execute the query you provided using linq to object, in order to get the expected results. Problem is that DataLength doesn't work in Linq to objects.

For those cases we need to provide expected query that can be executed on L2O.

here is an example:

            await AssertQuery(
                async,
                ss => ss.Set<Mission>().Where(g => g.Id > EF.Functions.DataLength(g.CodeName)),
                ss => ss.Set<Mission>().Where(g => g.Id > g.CodeName.Length * 2));

[maumar]

Please also add test that projects the DataLength value, e.g.

            await AssertQueryScalar(
                async,
                ss => ss.Set<Mission>().Select(m => EF.Functions.DataLength(m.CodeName)),
                ss => ss.Set<Mission>().Select(m => (int?)(m.CodeName.Length * 2)));

Note that this currently throws. @smitpatel any suggestions where to properly do the compensation for type difference between int and long in this case?

[Marusyk]

I'm closing this PR because don't know how to add tests.
If this code works fine for string:

await AssertQueryScalar(
                async,
                ss => ss.Set<Mission>().Select(m => EF.Functions.DataLength(m.CodeName)),
                ss => ss.Set<Mission>().Select(m => (int?)(m.CodeName.Length * 2)));

then what about other types: double, decimal, DateTimeOffset, ...? How to calculate (int?)(m.CodeName.Length * 2) for decimal? For double it should be 8, but m => (int?)8 won't work.

So, thanks all for your review and sorry for waste your time.

[maumar]

@Marusyk If you are still interested, I can add the missing tests in the follow up PR. Just rebase on current master and add a test for string property and I will merge. Test infra is quite confusing if one is not familiar with it :(

[Marusyk]

I'll rebase. Thanks

[maumar]

Merged. @Marusyk thank you for the contribution!

[AndriySvyryd]

@maumar didn't squash 😢