Improve evaluateOnce() helper function #9708
Conversation
|
This pull request was exported from Phabricator. Differential Revision: D56962773 |
✅ Deploy Preview for meta-velox canceled.
|
There was a problem hiding this comment.
@pedroerp This is looking very nice. Thank you for improving this helper function. Some nits.
| /// physical type that represent the custom type (e.g. `CustomType<T>::type`). | ||
| /// For example, use `int64_t` to return TIMESTAMP_WITH_TIME_ZONE() | ||
| /// | ||
| /// Input parameters and output should always wrapped in an std::optional to |
There was a problem hiding this comment.
typos: Input and output parameters should always be wrapped ...
| /// return evaluateOnce<double>("exp(c0)", a); | ||
| /// } | ||
| /// EXPECT_EQ(1, exp(0)); | ||
| /// EXPECT_EQ(std::nullopt, exp(std::nullopt)); |
There was a problem hiding this comment.
When testing default-null-behavior simple functions, it is redundant to test null inputs because these are handled by the framework, not by the function. It would be nice to clarify here that null tests are needed only when testing functions with non-default null behavior. It would also be nice to provide a version of evaluateOne that doesn't require std::optional.
There was a problem hiding this comment.
I guess here the point is less to test that the default null behavior works (which must already be covered elsewhere), but that the function null behavior is correctly defined.
For example, if at some point someone changes a function definition and that changes (breaks) its semantic regarding nulls, you should have a test case to point that out. So IMHO, even if the function has default null behavior, you should still add a null in, null out check.
| /// } | ||
| /// | ||
| /// | ||
| /// If not specified, input logical types are derived from the C++ type |
There was a problem hiding this comment.
Is this also true for return type? Is std::string supported as return type?
There was a problem hiding this comment.
added a sentence to clarify.
| /// | ||
| /// | ||
| /// One can also manually specify additional template parameters, in case | ||
| /// there are problems with template type deduction: |
There was a problem hiding this comment.
What would be an example of such a problem?
There was a problem hiding this comment.
for example, if the compiler cannot detect the full type, or if you want to a cast literal precision. Something like:
evaluateOnce<int64_t>("bla(c0, c1)", "my input char *", 1);
but this would:
evaluateOnce<int64_t, std::string, int64_t>("bla(c0, c1)", "my input char *", 1);
In most places I saw this being used, it wasn't really required, but this may come handy if you want to template test cases
| std::optional<TReturn> evaluateOnce( | ||
| const std::string& expr, | ||
| const TypePtr& type, | ||
| std::optional<TArgs>... args) { |
There was a problem hiding this comment.
Do we support more that one argument here? If so, does 'type' apply only to first argument? It would be nice to clarify.
There was a problem hiding this comment.
This overload is only here so you can do:
return evaluateOnce<int64_t>("hour(c0)", DATE(), date)
instead of:
return evaluateOnce<int64_t>("hour(c0)", {DATE()}, date)
I'll add a small comment to clarify.
There was a problem hiding this comment.
Do we support more that one argument here?
Yes, through the first overloaded version of this function (pass the types in the initializer_list)
| const std::string& expr, | ||
| const RowVectorPtr rowVectorPtr, | ||
| const std::optional<SelectivityVector>& rows = std::nullopt, | ||
| const TypePtr& resultType = nullptr) { | ||
| auto result = | ||
| evaluate<SimpleVector<facebook::velox::test::EvalType<ReturnType>>>( | ||
| evaluate<SimpleVector<facebook::velox::test::EvalType<TReturn>>>( |
There was a problem hiding this comment.
Perhaps, clarify somewhere that TReturn must be std::string for VARCHAR, VARBINARY and other typed based on StringView.
There was a problem hiding this comment.
Good point. Added a sentence.
Summary: Improving the evaluateOnce() helper function to allow for more flexibility when specifying input logical types. Also deprecating an old overload of the function that only allowed multiple parameters with they had the same type. . For a lot more details, check the header documentation added. Differential Revision: D56962773
|
This pull request was exported from Phabricator. Differential Revision: D56962773 |
Summary: Improving the evaluateOnce() helper function to allow for more flexibility when specifying input logical types. Also deprecating an old overload of the function that only allowed multiple parameters with they had the same type. . For a lot more details, check the header documentation added. Reviewed By: mbasmanova Differential Revision: D56962773
|
This pull request was exported from Phabricator. Differential Revision: D56962773 |
Summary: Improving the evaluateOnce() helper function to allow for more flexibility when specifying input logical types. Also deprecating an old overload of the function that only allowed multiple parameters with they had the same type. . For a lot more details, check the header documentation added. Reviewed By: mbasmanova Differential Revision: D56962773
|
This pull request was exported from Phabricator. Differential Revision: D56962773 |
Summary: Improving the evaluateOnce() helper function to allow for more flexibility when specifying input logical types. Also deprecating an old overload of the function that only allowed multiple parameters with they had the same type. . For a lot more details, check the header documentation added. Reviewed By: mbasmanova Differential Revision: D56962773
|
This pull request was exported from Phabricator. Differential Revision: D56962773 |
|
This pull request has been merged in ec4d2ec. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Summary: Pull Request resolved: facebookincubator#9708 Improving the evaluateOnce() helper function to allow for more flexibility when specifying input logical types. Also deprecating an old overload of the function that only allowed multiple parameters with they had the same type. . For a lot more details, check the header documentation added. Reviewed By: mbasmanova Differential Revision: D56962773 fbshipit-source-id: 3b16756ff6443275f08c401ccccff81e3aa1c399
Summary: Pull Request resolved: facebookincubator#9708 Improving the evaluateOnce() helper function to allow for more flexibility when specifying input logical types. Also deprecating an old overload of the function that only allowed multiple parameters with they had the same type. . For a lot more details, check the header documentation added. Reviewed By: mbasmanova Differential Revision: D56962773 fbshipit-source-id: 3b16756ff6443275f08c401ccccff81e3aa1c399
Summary:
Improving the evaluateOnce() helper function to allow for more
flexibility when specifying input logical types. Also deprecating an old
overload of the function that only allowed multiple parameters with they had
the same type.
.
For a lot more details, check the header documentation added.
Differential Revision: D56962773