-
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
Implemented type casting #662
Conversation
@@ -40,11 +40,11 @@ TEST_F(YieldTest, Basic) { | |||
} | |||
{ | |||
cpp2::ExecutionResponse resp; | |||
std::string query = "YIELD 1+1"; | |||
std::string query = "YIELD 1+1, '1+1', (int)3.14, (string)(1+1), (string)true"; |
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.
1+1 < 3
Unit testing passed. |
src/common/filter/Expressions.h
Outdated
return boost::get<int64_t>(value); | ||
case 1: | ||
return static_cast<int64_t>(boost::get<double>(value)); | ||
case 3: |
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.
case 2 and case 3
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.
Shame on me.
switch (value.which()) { | ||
case 0: | ||
return folly::to<std::string>(boost::get<int64_t>(value)); | ||
case 1: |
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 not use micro?
Unit testing passed. |
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.
Well done
Unit testing passed. |
Unit testing failed. |
Jenkins, go |
Unit testing passed. |
return boost::get<bool>(value) ? 1.0 : 0.0; | ||
case 3: | ||
// TODO(dutor) error handling | ||
return folly::to<double>(boost::get<std::string>(value)); |
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 think folly::tryTo would be better here, or exception would occur if an unparseable string is provided.
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 will address such problems after #669 is merged.
src/common/filter/Expressions.h
Outdated
case 2: | ||
return boost::get<bool>(value) ? 1.0 : 0.0; | ||
case 3: | ||
return boost::get<int64_t>(value); |
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.
We should do type casting here, right?
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.
Yes, this was a big mistake.
Unit testing passed. |
@CPWstatic I had resolved the conflicts. But the error handling work mentioned above is not addressed. I am going to do it in the next PR, since there are some additional work to do. |
Unit testing passed. |
static std::string toString(const VariantType &value) { | ||
char buf[1024]; | ||
switch (value.which()) { | ||
case 0: |
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.
Using the macro from Base.h
Unit testing passed. |
* Implemented type casting * Address @laura-ding's comments * Address comments
* Implemented type casting * Address @laura-ding's comments * Address comments
* invalidate empty key cache * address comment Co-authored-by: Doodle <13706157+critical27@users.noreply.github.com> Co-authored-by: Sophie <84560950+Sophie-Xie@users.noreply.github.com>
This pr implements and fixes #637