Skip to content
This repository has been archived by the owner on Dec 1, 2022. It is now read-only.

Expression could return Value directly and don't need to set the eval result to the private data member result_( #594

Closed
jievince opened this issue Jul 26, 2021 · 1 comment

Comments

@jievince
Copy link
Contributor

jievince commented Jul 26, 2021

Currently, eval() of expression returns a const value&. And most expressions store the result that evaled to a result_ and then return it to the caller. The caller most often picks it up with a copy of the value.

virtual const Value& eval(ExpressionContext& ctx) = 0;
 Value val = col->expr()->eval(ctx(iter.get()));

So this caused two copy overhead.
The reason why most of the callers use copy of value to pick the evaled result is some of the expressions don't store the evaled result to result_:

const Value& InputPropertyExpression::eval(ExpressionContext& ctx) {
    return ctx.getInputProp(prop_); // The value returned comes from the valueMap of ExecutionContext, so it shouldn't be moved by the caller
}

Moreover, it is a bit strange to store the evaled result that is only valid for the current row to the expression class.

We could let eval() return a Value and don't need to store the evaled result to result_.
Meanwhile, AggregateExpression is special. It's not stateless, so it should store the evaled result to result_.

@CPWstatic
Copy link
Contributor

We had a benchmark for these two situation.

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

No branches or pull requests

2 participants