-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
make evaluation of inner var expr thread-safe #4913
Conversation
b1628e8
to
e32606e
Compare
{listElement, ConstantExpression::make(pool_, static_cast<int64_t>(i))}); | ||
auto *nodeValue = VariableExpression::make(pool_, node->alias()); | ||
// The alias of node is converted to a inner variable. | ||
// e.g. MATCH (v:player) WHERE [t in [v] | (v)-[:like]->(t)] RETURN v | ||
// More cases could be found in PathExprRefLocalVariable.feature | ||
auto *nodeValue = VariableExpression::makeInner(pool_, node->alias()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node->alias() is converted to a inner var expr here.
@@ -316,6 +324,9 @@ class StorageExpressionContext final : public ExpressionContext { | |||
|
|||
// name -> Value with multiple versions | |||
std::unordered_map<std::string, std::vector<Value>> valueMap_; | |||
|
|||
// Expression value map that stores the value of innerVar | |||
std::unordered_map<std::string, Value> exprValueMap_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it MT-safe? And I don't see you create different ExprCtx
for each thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See
auto scatter = [this](size_t begin, size_t end, Iterator *tmpIter) -> StatusOr<DataSet> { |
Every thread has its own QueryExpressionContext.
Codecov ReportBase: 76.77% // Head: 76.84% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #4913 +/- ##
==========================================
+ Coverage 76.77% 76.84% +0.06%
==========================================
Files 1102 1102
Lines 81064 81091 +27
==========================================
+ Hits 62240 62316 +76
+ Misses 18824 18775 -49
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What type of PR is this?
What problem(s) does this PR solve?
Issue(s) number:
Fix #4844
Description:
How do you solve it?
Special notes for your reviewer, ex. impact of this fix, design document, etc:
Checklist:
Tests:
Affects:
Release notes:
Please confirm whether to be reflected in release notes and how to describe: