-
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
Fix wrong output when using pattern expression as the filter in MATCH #4788
Conversation
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.
Good job.
Value val = col->eval(ctx(probeIter)); | ||
// Clone the expression so when evaluating the InputPropertyExpression, the propIndex_ will | ||
// not be cached | ||
auto* colCopy = col->clone(); |
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.
The header of input table is fixed, why propIndex_
will be incorrect?
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.
For example, the column t
is the 3rd column in the left iterator and is the 2nd column in the right iterator.
I20221026 16:00:58.585834 2228773 RollUpApplyExecutor.cpp:39] left iter size: 7
I20221026 16:00:58.585844 2228773 RollUpApplyExecutor.cpp:40] left iter value: v|e|t|p|
I20221026 16:00:58.586164 2228773 RollUpApplyExecutor.cpp:42] right iter size: 6
I20221026 16:00:58.586175 2228773 RollUpApplyExecutor.cpp:43] right iter value: v|t|(v)-->(t:player)|
|
This is correct, but the |
Yes, that's the point of this bug, so don't need to clone expr in each row. |
I think you better clone in |
Moved the copy into |
@@ -143,25 +144,33 @@ folly::Future<Status> RollUpApplyExecutor::rollUpApply() { | |||
DataSet result; | |||
mv_ = movable(node()->inputVars()[0]); | |||
|
|||
if (rollUpApplyNode->compareCols().size() == 0) { | |||
auto compareCols = rollUpApplyNode->compareCols(); |
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, but this copy is useless.
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.
I'll remove this in another bug fix PR.
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.
Looks good.
What type of PR is this?
What problem(s) does this PR solve?
Issue(s) number:
Close #4760
Description:
The
roolUp
executor produces wrong results.After fix:
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: