Separate and move testCast function #7912
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
| const std::string& typeString, | ||
| std::vector<std::optional<TFrom>> input, | ||
| std::vector<std::optional<TTo>> expectedResult, | ||
| bool tryCast = false, |
There was a problem hiding this comment.
tryCast is needed when test case wants to verify if error cases lead to null outputs.
There was a problem hiding this comment.
Can we remove tryCast parameter here and add another method: testTryCast. This will make tests a bit easier to read.
|
@mbasmanova Could you help review? Thanks. |
There was a problem hiding this comment.
@rui-mo Thank you for the refactoring. Looks nice % some nits.
| template <typename TFrom, typename TTo> | ||
| void testCast( | ||
| const std::string& typeString, | ||
| std::vector<std::optional<TFrom>> input, |
There was a problem hiding this comment.
const& for input and expectedResult
| const std::string& typeString, | ||
| std::vector<std::optional<TFrom>> input, | ||
| std::vector<std::optional<TTo>> expectedResult, | ||
| bool tryCast = false, |
There was a problem hiding this comment.
Can we remove tryCast parameter here and add another method: testTryCast. This will make tests a bit easier to read.
| template <typename TFrom, typename TTo> | ||
| void testInvalidCast( | ||
| const std::string& typeString, | ||
| std::vector<std::optional<TFrom>> input, |
| std::vector<std::optional<TFrom>> input, | ||
| const std::string& expectedErrorMessage, | ||
| const TypePtr& fromType = CppToType<TFrom>::create(), | ||
| const TypePtr& toType = CppToType<TTo>::create()) { |
There was a problem hiding this comment.
toType parameter is not used
| fmt::format("cast(c0 as {})", typeString), | ||
| createRowVector<TFrom>(input, fromType)), | ||
| expectedErrorMessage); | ||
| return; |
|
|
||
| private: | ||
| template <typename T> | ||
| RowVectorPtr createRowVector( |
There was a problem hiding this comment.
Can you use make makeRowVector({makeFlatVector(input, fromType)})?
|
@mbasmanova Thanks for your review. Above comments were fixed. |
| const TypePtr& fromType = CppToType<TFrom>::create(), | ||
| const TypePtr& toType = CppToType<TTo>::create()) { | ||
| auto result = evaluate( | ||
| "cast(c0 as " + typeString + ")", |
| const TypePtr& fromType = CppToType<TFrom>::create(), | ||
| const TypePtr& toType = CppToType<TTo>::create()) { | ||
| auto result = evaluate( | ||
| "try_cast(c0 as " + typeString + ")", |
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mbasmanova has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@mbasmanova merged this pull request in bf6c3c1. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Moves
testCastfunction from CastExprTest.cpp to CastBaseTest.h for othertest files to use. Separates
testCastfunction intotestCast(for validoutput test),
testTryCast(for try_cast test), andtestInvalidCast(forexception test).
#7377 (comment)
#7377 (comment)