-
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
Refactor setOutputVar() to enforce the assignment of colNames explicitly. #4920
Conversation
223b430
to
167dcd6
Compare
Emmm. The column names of plan node are bind to output variable. This PR will fix some errors but lead to the others, in my mind. |
What other errors? |
Emm, not sure. |
3be9ca3
to
93c21a5
Compare
93c21a5
to
93dc0f6
Compare
…colNames explicitly.
93dc0f6
to
f86422a
Compare
Codecov ReportBase: 76.86% // Head: 76.90% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #4920 +/- ##
==========================================
+ Coverage 76.86% 76.90% +0.04%
==========================================
Files 1105 1105
Lines 81432 81444 +12
==========================================
+ Hits 62594 62638 +44
+ Misses 18838 18806 -32
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. |
@@ -221,7 +221,7 @@ class PlanNode { | |||
return numDeps() == 2U; | |||
} | |||
|
|||
void setOutputVar(const std::string& var); | |||
void setOutputVar(const std::string& var, const std::vector<std::string>& colNames); |
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.
better to give the colNames parameter a default value, such as empty vector, so you don't have to update the usage of this function all the codebase <:
In this PR, I've checked all assignments of colNames. No bugs found. Considering that this PR changed too many files, I close it for now. |
What type of PR is this?
What problem(s) does this PR solve?
Issue(s) number:
Close #4919
Close #4881
Description:
There is a risk that
colNames
are borrowed from a outputVar by mistake.How do you solve it?
Change the method's input parameters to require
colNames
explicitly.Changing
to
lhs->setOutputVar("var5", {"v2", "e2", "v3"});
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: