Fix #2319 caching of constant Field expressions #2321
Conversation
| @@ -14,7 +14,9 @@ namespace Nest | |||
| { | |||
| internal class HasConstantExpressionVisitor : ExpressionVisitor | |||
| { | |||
| public bool Found { get; private set; } | |||
| public bool Found => this.FoundConstants.Any(b => b); | |||
There was a problem hiding this comment.
.Count > 0 since its an ICollection
There was a problem hiding this comment.
We're checking for any true values within the collection, so Count > 0 isn't enough.
There was a problem hiding this comment.
Unless we only conditionally add to the collection...
There was a problem hiding this comment.
You're right, i wonder if we can just make it a bool flag that once set is never unset? Would safe on allocs too.
There was a problem hiding this comment.
Yea, good call. No need to maintain a collection at all, as long as we still visit each method call then we can use just the Found flag. I've changed this with my last commit.
|
Lgtm 👍 |
|
LGTM 👍 |
|
Shouldn't the expression visitor be renamed HasNonConstantExpressionVisitor? |
| @@ -35,7 +41,7 @@ protected override Expression VisitMethodCall(MethodCallExpression node) | |||
| var lastArg = node.Arguments.Last(); | |||
| var constantExpression = lastArg as ConstantExpression; | |||
| this.Found = constantExpression == null; | |||
| return node; | |||
There was a problem hiding this comment.
This can be left in to prevent traversing inner nodes unnecessarily.
There was a problem hiding this comment.
I don't think so. We need to actually visit all of the inner nodes to ensure there are only constants in the expression.
| @@ -50,16 +56,9 @@ protected override Expression VisitMethodCall(MethodCallExpression node) | |||
| var lastArg = node.Arguments.Last(); | |||
| var constantExpression = lastArg as ConstantExpression; | |||
| this.Found = constantExpression == null; | |||
| return node; | |||
There was a problem hiding this comment.
This can be left in to prevent traversing inner nodes unnecessarily.
We only cache Field expressions that do not contain variables. For
instance:
Field<Project>(p => p.Name)
Field<Project>(p => p.Name.Suffix("foo"))
Field<Project>(p => p.Metadata["foo"])
are all cachable since they only contain constant expressions.
whereas
Field<Project>(p => p.Name.Suffix(myVariable))
Field<Project>(p => p.Metadata[myVariable])
are not cachable since we can't evaluate the value of `myVariable` to be
used as a cache key.
However, there was a bug which this commit fixes where we would cache a
variable expression that ended with a constant. For instance:
Field<Project>(p => p.Metadata[myVariable].Suffix("foo"))
This expression was incorrectly cached because HasConstantExpressionVisitor
only concerned itself with the last method call: `Suffix("foo")`.
With this change, HasConstantExpressionVisitor has been renamed to
HasVariableExpressionVisitor to better align with what it actually
does. And now accounts for every node in the expression, ensuring that
we only cache expressions that contain only constants.
@brandondahler Agreed, it was confusing, I've renamed it to |
|
Ported to 5.x and master. |
We only cache Field expressions that do not contain variables. For
instance:
are all cachable since they only contain constant expressions.
whereas
are not cachable since we can't evaluate the value of
myVariableto beused as a cache key.
However, there was a bug which this commit fixes where we would cache a
variable expression that ended with a constant. For instance:
This expression was incorrectly cached because HasConstantExpressionVisitor
only concerned itself with the last method call:
Suffix("foo").With this change,
HasConstantExpressionVisitorhas been renamed toHasVariableExpressionVisitorto better align with what it actuallydoes. And now accounts for every node in the expression, ensuring that
we only cache expressions that contain only constants.