Add support for queryable functions #19575

pmiddleton · 2020-01-12T19:29:25Z

Update of queryable functions to support the new 3.x query pipeline.

Addresses part of #4319 and allows functions to be used as query roots in linq queries.

Todo - documentation with examples.

ajcvickers · 2020-01-13T22:24:03Z

@pmiddleton Thanks for sending this PR. Unfortunately, @smitpatel is currently out on vacation, and he needs to review this before we can merge, so please forgive us if things are a bit slow here.

@AndriySvyryd @bricelam will take a quick look in the meantime, but we'll need to wait for Smit before merging.

pmiddleton · 2020-01-13T22:44:01Z

@ajcvickers Sounds good.

Hopefully @smitpatel will be well rested when he gets back so he can go easy on my changes 😁

AndriySvyryd · 2020-01-14T08:27:36Z

src/EFCore.Relational/Metadata/Internal/DbFunction.cs

+                {
+                    model.AddEntityType(methodInfo.ReturnType.GetGenericArguments()[0]).SetAnnotation(RelationalAnnotationNames.QueryableFunctionResultType, null);
+                }
+            }


I don't think we need a new attribute for this type of entity type. Instead it can be discovered by RelationalDbFunctionAttributeConvention as a keyless entity type mapped to a view named after the function. Then stored in DbFunction as IQueryableEntityType property. Model validation would then check that it's not null for an IQueryable<> IDbFunction.

I undetstand that QueryableFunctionResultType is a bit of a hack per a conversation I had with @bricelam. #2725 is needed for the real fix.

The code does support keyed result types which are not full entities (no table wanted). If there is a valid key we can use it to project the results into a collection. Views only support keyless types so going that route will break that feature.

We already have a hack in place, that's the view mapping. The entity type doesn't need to be keyless to be mapped to a view, so if keyed entities are supported the convention doesn't need to configure them as keyless. If it is indeed keyless the user will be able to configure it afterwards.

Ok so if I am following you - you are saying move the entity registration into RelationalDbFunctionAttributeConvention and use .Toview instead.

I am not following the need for storing an IQueryableEntityType in DbFunction Model validation currently checks that the result type is mapped as an entity for QueryableFunctions.

If you just use the CLR type to lookup the entity type you won't be able to use weak nor shared type entity types. However this would be useful if you have more than one function returning the same type or if you actually want to also map it to a table.

I've been working on #12846 and changed my mind on how this should be implemented. Instead the mapped functions should be stored in an annotation on the entity type. And yes, you would need to look through all entity types to find the corresponding one for a given function, but once #12846 is committed it will make this fast. It will also add a non-hacky way of not creating a table in migrations.

Moreover multiple functions can be mapped to the same entity type, so it would be better to configure the mapping from the function side. Also a function can be designated as the default mapping so you can query using the entity type as root (e.g. context.TopSellingProducts.ToList()).

Perhaps we could also allow mapping an entity type to a function without having to add a method for it.

Ill keep my eye on #12846 Is there a time frame of when this work will be done? I am holding off on making any code changes for now until I get feedback from @smitpatel on the PR.

#12846 Should be mostly done this week (unless something unexpected comes up). Note that the relational model is built after the base model is validated, so we'll still need all the annotations to store the configuration up to that point at least.

src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs

src/EFCore.Relational/Metadata/Internal/DbFunction.cs

src/EFCore.Relational/Query/ISqlExpressionFactory.cs

smitpatel · 2020-02-05T17:00:00Z

The mis-spelling of queryable! ouch!

pmiddleton · 2020-02-05T17:59:38Z

@smitpatel - yea you're back. Hope you had a good vacation.

Oh no I thought I caught all the places where I fat fingered that in the code.

pmiddleton · 2020-02-05T18:00:58Z

Sigh.... nope... and I even did it in the pr title.... ugh.

Ok I'll get those cleaned up tonight.

pmiddleton · 2020-02-05T18:07:41Z

Ok it was bugging me too much - I cleaned up that rename now. I will fix the conflicting file tonight.

smitpatel · 2020-02-10T19:46:36Z

test/EFCore.Relational.Specification.Tests/Query/UdfDbFunctionTestBase.cs

+                return CreateQuery(() => GetCustomerOrderCountByYear(customerId));
+            }
+
+            public IQueryable<OrderByYear> GetCustomerOrderCountByYear(Expression<Func<int>> customerId2)


Is there any specific value on supporting this?

If you want to pass a scalar function as a parameter to a queryable function then it needs to be passed as an expression. Otherwise it will be evaluated locally.

See QF_Stand_Alone_Nested for an example.

Is this going to be a big use case - probably not, but it didn't require any extra code in the backend. It just works so I tossed in a unit test.

If we are looking to support lambdas then probably should take parameter lambda too rather than just parameter-less.
Btw does using let in the query and passing that work?

P.S. this lambda reminds me of Skip/Take with lambda we had to introduce in EF6 for parameterization.

Are you talking about the specific method that takes the Expression

GetCustomerOrderCountByYear(Expression<Func<int>> customerId2)

or the fact that the body has the lambda in it?

return CreateQuery(() => GetCustomerOrderCountByYear(customerId));

All Queryable Functions will be executed at runtime, there is no way to prevent that based on how linq works. We need to convert that call into an IQueryable which can then be built into the Expression Tree.

The way I achieve that is by passing into CreateQuery a lambda with the function call to itself. That in turn gets passed to ContextDependencies.QueryProvider.CreateQuery which returns an IQueryable that can get passed back to linq.

That seemed like the most user friendly way for the end users to build the Expression we need to pass to ContextDependencies.QueryProvider.CreateQuery. I'm open to other ideas.

the method taking Expression<Func>

I'm not sure if let works. Ill have to think of a test case and then give it a try.

smitpatel · 2020-02-21T23:09:57Z

@pmiddleton - The time for this PR will come along with #18923. There are quite a few refactoring in pipeline which conflicts with this (like navexpansionfactory). I reviewed it and most of the code is right place. There are design questions which we as a team need to discuss, determine API, feature set we want to support for initial release. Though I wouldn't want you to keep waiting for this to get merged for months. I can take over from this point onward.

pmiddleton · 2020-02-22T00:42:53Z

@smitpatel - I first started this feature 4 years ago, so a few more months won't bother me :)

That being said if it is easier for you to make changes to this feature as you make changes for #18923 then go for it. Otherwise I'll be around to make changes to keep it working if you need the extra bandwidth.

smitpatel · 2020-02-25T00:28:56Z

I did mess up something in git 🙄

AndriySvyryd · 2020-02-25T00:33:33Z

@smitpatel Perhaps just submit a new PR?

smitpatel · 2020-02-25T00:36:17Z

Of course, after I wiped out all changes here. #NeverForcePush

smitpatel · 2020-03-04T01:27:58Z

The DbContext from queryable function leaves behind in the tree. This puts DbContext context in query cache causing leak. 😭

pmiddleton · 2020-03-04T01:56:24Z

Wont replacing the function calls with a custom expression, as mention in the other or, break the link?

smitpatel · 2020-03-04T02:27:12Z

@pmiddleton - This is before parameter extraction phase so other may or may not affect it. I filed #20156 to track the issue. I noticed difference in SQL generated when I changed a little thing in parameter extraction during #20154. Interestingly that tiny change also fixed the issue. I filed issue to see if there is any point of creating parameter for DbContext or should we just null it out during funcletization.

pmiddleton force-pushed the queryableFunctions branch from 092771f to bb7edb6 Compare January 13, 2020 05:50

AndriySvyryd reviewed Jan 14, 2020

View reviewed changes

Suchiman reviewed Jan 14, 2020

View reviewed changes

src/EFCore.Relational/Infrastructure/RelationalModelValidator.cs Outdated Show resolved Hide resolved

Suchiman reviewed Jan 14, 2020

View reviewed changes

src/EFCore.Relational/Metadata/Internal/DbFunction.cs Outdated Show resolved Hide resolved

Suchiman reviewed Jan 14, 2020

View reviewed changes

src/EFCore.Relational/Query/ISqlExpressionFactory.cs Outdated Show resolved Hide resolved

pmiddleton force-pushed the queryableFunctions branch from bb7edb6 to e79dbd7 Compare January 14, 2020 17:14

ajcvickers assigned smitpatel Jan 17, 2020

pmiddleton force-pushed the queryableFunctions branch from e79dbd7 to cbeb5ec Compare January 19, 2020 23:43

pmiddleton changed the title ~~Add support for querable functions~~ Add support for queryable functions Feb 5, 2020

pmiddleton force-pushed the queryableFunctions branch from cbeb5ec to 9d23f6c Compare February 5, 2020 18:07

pmiddleton force-pushed the queryableFunctions branch from 9d23f6c to ad8b077 Compare February 8, 2020 21:09

smitpatel reviewed Feb 10, 2020

View reviewed changes

smitpatel linked an issue Feb 25, 2020 that may be closed by this pull request

Ability to map a CLR method returning queryable to TVF #20051

Closed

10 tasks

pmiddleton requested a review from dougbu as a code owner February 25, 2020 00:27

smitpatel closed this Feb 25, 2020

smitpatel force-pushed the queryableFunctions branch from 68d183a to 23f9bde Compare February 25, 2020 00:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for queryable functions #19575

Add support for queryable functions #19575

pmiddleton commented Jan 12, 2020 •

edited

Loading

ajcvickers commented Jan 13, 2020

pmiddleton commented Jan 13, 2020

AndriySvyryd Jan 14, 2020

pmiddleton Jan 14, 2020

AndriySvyryd Jan 14, 2020

pmiddleton Jan 14, 2020

AndriySvyryd Jan 14, 2020 •

edited

Loading

AndriySvyryd Feb 5, 2020 •

edited

Loading

pmiddleton Feb 9, 2020

AndriySvyryd Feb 9, 2020

smitpatel commented Feb 5, 2020

pmiddleton commented Feb 5, 2020

pmiddleton commented Feb 5, 2020

pmiddleton commented Feb 5, 2020

smitpatel Feb 10, 2020

pmiddleton Feb 12, 2020

smitpatel Feb 12, 2020

pmiddleton Feb 12, 2020

smitpatel Feb 13, 2020

pmiddleton Feb 17, 2020

smitpatel commented Feb 21, 2020

pmiddleton commented Feb 22, 2020

smitpatel commented Feb 25, 2020

AndriySvyryd commented Feb 25, 2020

smitpatel commented Feb 25, 2020

smitpatel commented Mar 4, 2020

pmiddleton commented Mar 4, 2020

smitpatel commented Mar 4, 2020

Add support for queryable functions #19575

Add support for queryable functions #19575

Conversation

pmiddleton commented Jan 12, 2020 • edited Loading

ajcvickers commented Jan 13, 2020

pmiddleton commented Jan 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndriySvyryd Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

AndriySvyryd Feb 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel commented Feb 5, 2020

pmiddleton commented Feb 5, 2020

pmiddleton commented Feb 5, 2020

pmiddleton commented Feb 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smitpatel commented Feb 21, 2020

pmiddleton commented Feb 22, 2020

smitpatel commented Feb 25, 2020

AndriySvyryd commented Feb 25, 2020

smitpatel commented Feb 25, 2020

smitpatel commented Mar 4, 2020

pmiddleton commented Mar 4, 2020

smitpatel commented Mar 4, 2020

pmiddleton commented Jan 12, 2020 •

edited

Loading

AndriySvyryd Jan 14, 2020 •

edited

Loading

AndriySvyryd Feb 5, 2020 •

edited

Loading