Check determinism using function name only #10241
Conversation
✅ Deploy Preview for meta-velox canceled.
|
c2cea96 to
e876e68
Compare
11808fe to
3abd0e2
Compare
|
@mbasmanova Would you help review this PR? It addresses the discussion at #9149 (comment). Thanks. |
velox/expression/VectorFunction.h
Outdated
| /// no function with the specified name. | ||
| std::optional< | ||
| std::pair<std::vector<FunctionSignaturePtr>, VectorFunctionMetadata>> | ||
| getVectorFunctionSignaturesAndMetadata(const std::string& name); |
There was a problem hiding this comment.
Looks like the only caller of this API uses only the metadata. Perhaps, change the API to return just that:
std::optional<VectorFunctionMetadata> getVectorFunctionMetadata(const std::string& name);
| @@ -26,6 +26,14 @@ | |||
| #include "velox/vector/tests/utils/VectorMaker.h" | |||
|
|
|||
| namespace facebook::velox::fuzzer { | |||
| namespace detail { | |||
There was a problem hiding this comment.
APIs in 'detail' namespaces are not supposed to be used outside of the header/cpp files.
I wonder if this API belongs to velox/functions/FunctionRegistry.h and whether it is clearer to return std::optional to indicate not-found condition clearly.
| /// Returns if `functionName` with the given `argTypes` is deterministic. | ||
| /// Returns true if the function was not found or determinism cannot be | ||
| /// established. | ||
| bool isDeterministic( |
There was a problem hiding this comment.
Let's keep this function, but remove 'argTypes' and replace code from L359 with a call to new isDeterministic(name) API. The new API would go into elox/functions/FunctionRegistry.h and return std::optional<bool>. std::nullopt would indicate that function not found, true would indicate that all function signatures are deterministic, false would mean that at least one function signature is non-deterministic.
What do you think?
There was a problem hiding this comment.
Thanks for the suggestion. Added a new isDeterministic API in FunctionRegistry.h.
c183362 to
0046a88
Compare
| // get their determinism. | ||
| std::vector<TypePtr> argTypes; | ||
| if (signature->variables().empty()) { | ||
| if (!isDeterministic(function.first)) { |
There was a problem hiding this comment.
No need to change anything in this PR, but I wonder if we can move this check into filterSignatures now that the check only needs function name.
There was a problem hiding this comment.
I see. Will follow up this change in a separate PR. Thanks.
|
@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@bikramSingh91 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@bikramSingh91 merged this pull request in 63cceca. |
|
Conbench analyzed the 1 benchmark run on commit There were no benchmark performance regressions. 🎉 The full Conbench report has more details. |
Summary: The check for determinism could be moved into filterSignatures as it only needs function name. #10241 (comment) Pull Request resolved: #10394 Reviewed By: bikramSingh91 Differential Revision: D59592576 Pulled By: pedroerp fbshipit-source-id: 82b1673c4f0efdee7cfd6084daba98d96163c7d5
In ExpressionFuzzer, the determinism could be decided without argument
types. This PR changes to check determinism using function name only by
fetching all registry entries for the given function name and checking if all
of them are deterministic. The function is deterministic if "deterministic" is
true for every entry. With this change, argument types are not required when
the signature variables are not empty.