Added support for compute() transformation in $apply - #1877
Conversation
c15808b to
b38ea6c
Compare
madansr7
added
the
Ready for review
|
@kosinsky i re-based the PR on the latest master. It seems some test cases are failing. Would you please take a look if you think it's better to release in 7.2.2 release. #Closed |
7415d94 to
c7ffc77
Compare
|
When it will be merged and when available for getting from nuget? |
|
These features (compute() and Core 3.0) aren't related. And compute() implementation doesn't do anything that could affect Core 3.0 #Closed |
|
@KanishManuja-MS When will be available next release or pre-release? #Closed |
|
@KanishManuja-MS - I see that issue is still open. Can you please let me know when this will be available #Closed |
|
@VenuMadhavPerumalla We will try to get this in our next WebAPI release. Thanks! #Closed |
|
@KanishManuja-MS - could you advise when the next WebAPI release will be? We’d also find this functionality useful... #Closed |
|
@GitMonkey007 We don't have date put down as of now but I would imagine towards the end of the month or early February. In the meanwhile, I encourage you to review this PR to see if it meets all your needs. #Closed |
bcffaee to
18460a0
Compare
18460a0 to
d952eac
Compare
| case Linq2ObjectsQueryProviderNamespace: | ||
| // EF Core before 3.0 does a lot of client evaluations and has InMemory support which is the same as Linq2Objects | ||
| case ObjectContextQueryProviderNamespaceEFCore2: |
There was a problem hiding this comment.
Isn't this a breaking change? #Resolved
There was a problem hiding this comment.
Potentially. However, if you try to create expressions like $filter=length(Field) gt 10 when backed by EF InMemory it will throw with version of code that in master now. That's a bug.
When we generate IQueryable provided by EF and we have something like
x => x.Field.Length > 10we assumed that EF will pass that operation to the SQL in a form
LEN(Field) > 10If Field is null it will; work in SQL, but executed directly C# expression throw exception and we have to convert if to
x => x.Field == null ? false : x.Field.Length > 10We are doing it for linq to objects, but not EF InMemory #Resolved
There was a problem hiding this comment.
@mikepizzo Looks like we don't have tests for $filter and EF Core. Do you want me to:
- Create separate PR to add these tests
- Add these test to this PR
- Create an issue to work on extra EF Core tests after this PR is megred
There was a problem hiding this comment.
I'm fine with a separate PR for EF Core tests so that we don't hold up this PR any longer -- thanks!
| { | ||
| // Add real OData properties to the collection | ||
| // We need to use injected Model to real proeprty names | ||
| var edmType = GetEdmType() as IEdmEntityTypeReference; |
There was a problem hiding this comment.
IEdmEntityTypeReference [](start = 50, length = 23)
Will this cast always succeed, or might GetEdmType return something besides an IEdmEntityTypeReference?
What about complex types? #Resolved
There was a problem hiding this comment.
For this PR (adding compute() to $apply), it should be true all the time, because we are using only EntitySet?$apply=....
When we will work on $apply inside $expand (I have that feature in my fork). For queries like:
EntitySet?$expand=ComplexTypeCollection($apply=...) it will change. I think it will be really difficult to validate that ComputeWrapper works for complex types new before implementing $expand($apply) and/or $expand($compute) #Resolved
|
Hoping to get a beta release out next week, with RTM a couple of weeks after, depending on feedback. In reply to: 571371458 [](ancestors = 571371458) |
mikepizzo
left a comment
mikepizzo
left a comment
There was a problem hiding this comment.
A few minor comments, mostly regarding:
- Ability to apply Compute() to a complex type
- Would prefer to provide a cleaner mechanism for mutating state than exposing fields from base types
Also a few minor spelling issues, but other than that it looks good -- thanks, and apologies for taking so long to get this reviewed!
|
I'm not planning any additional changes. It's good to go as soon as CI completes. However, CI stuck in "waiting for status to be reported". Looks like build wasn't even started. |
|
Was able to run CI and verify, although pipeline was unable to post results back to GitHub. |
|
Working great for us so far! Thanks everyone!! :) |
|
How to get this feature ? |
* Added support for functions and constants in aggregate expression * Tests for more cannonical functions * Time tests * Support for compute transformation in $apply * Support sorting for columns introduced in compute * Remove redundant code * Fix flakyness in AggregateNavigationPropertyWorks * Fix the ToInt16 overflow during build * Fix typos * Make _values field private * Explicit way to mutate element type and lambda param * Test to demonstrate that we could run compute on complex entity properties * Support for complex types * Simplify keepoing ElementType and LambdaParameter in sync Co-authored-by: Sam Xu <saxu@microsoft.com>
* Added support for functions and constants in aggregate expression * Tests for more cannonical functions * Time tests * Support for compute transformation in $apply * Support sorting for columns introduced in compute * Remove redundant code * Fix flakyness in AggregateNavigationPropertyWorks * Fix the ToInt16 overflow during build * Fix typos * Make _values field private * Explicit way to mutate element type and lambda param * Test to demonstrate that we could run compute on complex entity properties * Support for complex types * Simplify keepoing ElementType and LambdaParameter in sync Co-authored-by: Sam Xu <saxu@microsoft.com>
9 participants
Description
Known limitation: compute() transformation must be used with groupby or aggregate. So we couldn't do
$apply=compute(1 as Test)that is the same as$compute=1 as Test. Limitation caused by the same problem as not supporting $select after $apply.Issues
Fixes scenario from issue #793
Checklist (Uncheck if it is not completed)