-
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
Check determinism using function name only #10241
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -341,12 +341,9 @@ static void appendSpecialForms( | |
} | ||
} | ||
|
||
/// Returns if `functionName` with the given `argTypes` is deterministic. | ||
/// Returns true if the function was not found or determinism cannot be | ||
/// established. | ||
bool isDeterministic( | ||
const std::string& functionName, | ||
const std::vector<TypePtr>& argTypes) { | ||
// Returns if `functionName` is deterministic. Returns true if the function was | ||
// not found or determinism cannot be established. | ||
bool isDeterministic(const std::string& functionName) { | ||
// We know that the 'cast', 'and', and 'or' special forms are deterministic. | ||
// Hard-code them here because they are not real functions and hence cannot | ||
// be resolved by the code below. | ||
|
@@ -356,15 +353,14 @@ bool isDeterministic( | |
return true; | ||
} | ||
|
||
if (auto typeAndMetadata = | ||
resolveFunctionWithMetadata(functionName, argTypes)) { | ||
return typeAndMetadata->second.deterministic; | ||
const auto determinism = velox::isDeterministic(functionName); | ||
if (!determinism.has_value()) { | ||
// functionName must be a special form. | ||
LOG(WARNING) << "Unable to determine if '" << functionName | ||
<< "' is deterministic or not. Assuming it is."; | ||
return true; | ||
} | ||
|
||
// functionName must be a special form. | ||
LOG(WARNING) << "Unable to determine if '" << functionName | ||
<< "' is deterministic or not. Assuming it is."; | ||
return true; | ||
return determinism.value(); | ||
} | ||
|
||
std::optional<CallableSignature> processConcreteSignature( | ||
|
@@ -567,13 +563,26 @@ ExpressionFuzzer::ExpressionFuzzer( | |
continue; | ||
} | ||
|
||
// Determine a list of concrete argument types that can bind to the | ||
// signature. For non-parameterized signatures, these argument types will | ||
// be used to create a callable signature. For parameterized signatures, | ||
// these argument types are only used to fetch the function instance to | ||
// get their determinism. | ||
std::vector<TypePtr> argTypes; | ||
if (signature->variables().empty()) { | ||
if (!isDeterministic(function.first)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Will follow up this change in a separate PR. Thanks. |
||
LOG(WARNING) << "Skipping non-deterministic function: " | ||
<< function.first << signature->toString(); | ||
continue; | ||
} | ||
|
||
if (!signature->variables().empty()) { | ||
std::unordered_set<std::string> typeVariables; | ||
for (const auto& [name, _] : signature->variables()) { | ||
typeVariables.insert(name); | ||
} | ||
atLeastOneSupported = true; | ||
++supportedFunctionSignatures; | ||
signatureTemplates_.emplace_back(SignatureTemplate{ | ||
function.first, signature, std::move(typeVariables)}); | ||
} else { | ||
// Determine a list of concrete argument types that can bind to the | ||
// signature. For non-parameterized signatures, these argument types | ||
// will be used to create a callable signature. | ||
std::vector<TypePtr> argTypes; | ||
bool supportedSignature = true; | ||
for (const auto& arg : signature->argumentTypes()) { | ||
auto resolvedType = SignatureBinder::tryResolveType(arg, {}, {}); | ||
|
@@ -589,37 +598,15 @@ ExpressionFuzzer::ExpressionFuzzer( | |
<< function.first << signature->toString(); | ||
continue; | ||
} | ||
} else { | ||
ArgumentTypeFuzzer typeFuzzer{*signature, localRng}; | ||
typeFuzzer.fuzzReturnType(); | ||
VELOX_CHECK_EQ( | ||
typeFuzzer.fuzzArgumentTypes(options_.maxNumVarArgs), true); | ||
argTypes = typeFuzzer.argumentTypes(); | ||
} | ||
if (!isDeterministic(function.first, argTypes)) { | ||
LOG(WARNING) << "Skipping non-deterministic function: " | ||
<< function.first << signature->toString(); | ||
continue; | ||
} | ||
|
||
if (!signature->variables().empty()) { | ||
std::unordered_set<std::string> typeVariables; | ||
for (const auto& [name, _] : signature->variables()) { | ||
typeVariables.insert(name); | ||
if (auto callableFunction = processConcreteSignature( | ||
function.first, | ||
argTypes, | ||
*signature, | ||
options_.enableComplexTypes)) { | ||
atLeastOneSupported = true; | ||
++supportedFunctionSignatures; | ||
signatures_.emplace_back(*callableFunction); | ||
} | ||
atLeastOneSupported = true; | ||
++supportedFunctionSignatures; | ||
signatureTemplates_.emplace_back(SignatureTemplate{ | ||
function.first, signature, std::move(typeVariables)}); | ||
} else if ( | ||
auto callableFunction = processConcreteSignature( | ||
function.first, | ||
argTypes, | ||
*signature, | ||
options_.enableComplexTypes)) { | ||
atLeastOneSupported = true; | ||
++supportedFunctionSignatures; | ||
signatures_.emplace_back(*callableFunction); | ||
} | ||
} | ||
|
||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. Added a new isDeterministic API in FunctionRegistry.h.