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

WIP - Support non parameterized constants query plan caching for Linq provider #2375

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

maca88
Copy link
Contributor

@maca88 maca88 commented May 9, 2020

With this PR query plans for queries like:

db.Customers.Select(o => new {o.CustomerId, Constant = "constant"}).First()
db.Customers.WithLock(LockMode.Upgrade).ToList()

will be cached. This was achieved by adding an additional parameter for item/list/post transformers, which contains all parameter values.

For the following query:

db.Customers.Select(o => new {o.CustomerId, Constant = "constant"}).First()

the selector will be cached as:
(input, parameterValues) => new {CustomerId = input[0], Constant = parameterValues[0]}
before this PR it was cached as:
(input) => new {CustomerId = input[0], Constant = "constant"}.

In order to handle scenarios where a parameter detected by ExpressionParameterVisitor is converted to a hql constant, a special logic was added (VisitorParameters.UpdateCanCachePlan) that check whether all parameters in the expression were converted to hql. One example is TrimGenerator, where char parameter is converted to string and it is used to create a hql constant. In that case the query plan won't be cached.

Added WIP as it is a continuation of #2365.

Possible breaking changes:

  • Property AdditionalCriteria in ExpressionToHqlTranslationResults is not populated anymore, PreQueryExecuteDelegates property has to be used instead
  • Delegate ExpressionToHqlTranslationResults.PostExecuteTransformer gained one parameter of type object[]
  • Exceptions that occur when calling Linq ToFutureValue and ToFuture methods are no longer wrapped inside TargetInvocationException.

@maca88 maca88 force-pushed the ConstantParametersCaching branch 2 times, most recently from cb0b7ce to a86ccb3 Compare May 9, 2020 17:29

// TODO 6.0: Move to IResultTransformer
internal interface IResultTransformerExtended : IResultTransformer
Copy link
Member

@bahusoid bahusoid May 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This IResultTransformer interface is quite an ugly thing and this extension doesn't make it any better... Can you think of any way to avoid this interface changes? Why do we need to have parameterValues as parameters? Why can't it be internal state of LINQ ResultTransfomer class? IMHO it's better to recreate it on each query execution if it avoids interface changes (if for instance it's now part of query cache). Is it really needed in any other place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you think of any way to avoid this interface changes?

Maybe by creating a new instance of ResultTransformer each time a query is executed and pass parameterValues in the constructor instead. I will try if it is possible.

@maca88 maca88 force-pushed the ConstantParametersCaching branch from a86ccb3 to 2c9cc58 Compare May 9, 2020 21:29
@maca88 maca88 force-pushed the ConstantParametersCaching branch from 07de27f to ac6d753 Compare May 16, 2020 21:16
@maca88
Copy link
Contributor Author

maca88 commented May 16, 2020

Force pushed a version that does not need to alter the IResultTransformer interface (here is the old version). In the current state, a new ExpressionToHqlTranslationResults is created for each execution by using WithParameterValues method, which creates a new ResultTransformer and PostResultTransformer (added in this PR) instance that contain the parameter values. The newly added PostResultTransformer isn't invoked with DynamicInvoke anymore, as it is constructed as Func<object, object[], object>, so exceptions will not be wrapped inside TargetInvocationException anymore. This causes a possible breaking change for ToFutureValue and to ToFuture methods as exceptions were not unwrapped from a TargetInvocationException (Updated description).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants