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

Query: Allow certain unmapped constant in shaper expression #17048

Closed
smitpatel opened this issue Aug 9, 2019 · 2 comments · Fixed by #17142
Closed

Query: Allow certain unmapped constant in shaper expression #17048

smitpatel opened this issue Aug 9, 2019 · 2 comments · Fixed by #17142
Assignees
Labels
closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@smitpatel
Copy link
Contributor

smitpatel commented Aug 9, 2019

Fix for #13048 involves trying to infer the constant expression in client code which are not server correlated and may contain reference to enclosing class which in terms capture context causing memory leak.
The heuristic we are currently using is, constantExpression

  • whose value is not null
  • & which are not mapped by current provider (since we don't send constants to server during client eval).

The set of conditions also includes certain other cases here. While it may not be always accurate but probably can be improved. 2 Failing cases right now.

        [ConditionalTheory]
        [MemberData(nameof(IsAsyncData))]
        public virtual Task GroupBy_empty_key_Aggregate_Key(bool isAsync)
        {
            return AssertQuery<Order>(
                isAsync,
                os =>
                    os.GroupBy(
                            o => new
                            {
                            })
                        .Select(
                            g => new
                            {
                                g.Key,
                                Sum = g.Sum(o => o.OrderID)
                            }));
        }

Group by empty key and then projecting such key (it works if key is not used in projection). Here we receive constant of anonymous type.
We added these pattern for dev expression (ref #11905)
But we cannot find if the anonymous constant references enclosing class or not.
See also #15712 If that is fixed then perhaps we can fix this case.

[ConditionalTheory]
        [MemberData(nameof(IsAsyncData))]
        public virtual async Task ToString_with_formatter_is_evaluated_on_the_client(bool isAsync)
        {
            await AssertQuery<Order>(
                isAsync,
                os => os.Where(o => o.OrderDate != null)
                    .Select(
                        o => new Order
                        {
                            ShipName = o.OrderID.ToString(new CultureInfo("en-US"))
                        }),
                e => e.ShipName);
        }

CultureInfo is not mapped type. In future we could allow certain types we know not coming from client code to be in tree as we can be certain it cannot contain enclosing class.

@ajcvickers
Copy link
Contributor

Notes from triage: @smitpatel to look at fixing the first case, since this is important for control vendors. Beyond that we still want to revisit this post 3.0.

@ajcvickers ajcvickers added this to the 3.0.0 milestone Aug 12, 2019
smitpatel added a commit that referenced this issue Aug 14, 2019
…ression tree

This puts some of the processing (evaluating the expression to the corresponding constant) inside translation pipeline.
- When applying EntityEquality, assumption here is that property is always going to be mapped on server side so we can generate constant.
- When translating newExpression. If the generated constant can be mapped, it would work. (like new Datetime()) else translation would null out.

Resolves #15712
Resolves #17048
Resolves #7983
@smitpatel smitpatel added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label Aug 14, 2019
@smitpatel
Copy link
Contributor Author

Both of the cases worked using fix for #15712. So closing this alongwith.

smitpatel added a commit that referenced this issue Aug 14, 2019
…ression tree

This puts some of the processing (evaluating the expression to the corresponding constant) inside translation pipeline.
- When applying EntityEquality, assumption here is that property is always going to be mapped on server side so we can generate constant.
- When translating newExpression. If the generated constant can be mapped, it would work. (like new Datetime()) else translation would null out.

Resolves #15712
Resolves #17048
Resolves #7983
@ajcvickers ajcvickers modified the milestones: 3.0.0, 3.0.0-preview9 Aug 21, 2019
@ajcvickers ajcvickers modified the milestones: 3.0.0-preview9, 3.0.0 Nov 11, 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. type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants