-
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
Add unit tests for struct Converter #7884
Conversation
✅ Deploy Preview for meta-velox canceled.
|
20f5524
to
f4bbbbe
Compare
@gggrace14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
f4bbbbe
to
e26180a
Compare
@gggrace14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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. Thank you for adding the test coverage! The failed CircleCI signal should have been fixed. Please rebase to ensure the test passes.
velox/type/tests/ConversionsTest.cpp
Outdated
testConversion<float, int64_t>( | ||
{kInf}, {}, /*truncate*/ false, false, /*expectError*/ true); | ||
testConversion<float, int64_t>( | ||
{kNan}, {0}, /*truncate*/ false, false, /*expectError*/ 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.
nit: Can these two be combined into one testConversion call?
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.
Revised
velox/type/tests/ConversionsTest.cpp
Outdated
testConversion<float, int64_t>( | ||
{kInf}, {9223372036854775807}, /*truncate*/ true); | ||
testConversion<float, int64_t>({kNan}, {0}, /*truncate*/ 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.
Ditto.
Add unit tests for struct Converter and Conversions.h. struct Converter holds the core logic for casting between scalar types. Source of the test cases are CastExprTest.cpp and https://facebookincubator.github.io/velox/functions/presto/conversion.html
e26180a
to
cd408cd
Compare
@gggrace14 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Thank you, @kagamiori Wei, for the quick review! |
@gggrace14 merged this pull request in 7249cd9. |
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Add unit tests for struct Converter and Conversions.h. struct Converter
holds the core logic for casting between scalar types. Source of the test
cases are CastExprTest.cpp and
https://facebookincubator.github.io/velox/functions/presto/conversion.html