-
Notifications
You must be signed in to change notification settings - Fork 5.5k
misc(native): Remove the force restriction on function name prefix #26675
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
base: master
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR relaxes the strict requirement that Velox function names must have exactly three dot-separated components and instead treats non-3-part names as ignorable during metadata collection, removing the previous hard failure behavior and updating callers and tests accordingly. Updated class diagram for function metadata utilitiesclassDiagram
class Util {
+getFunctionNameParts(registeredFunction : std::string) std::vector<std::string>
}
class FunctionMetadata {
+getFunctionsMetadata(catalog : std::optional<std::string>) json
-skipCatalog(catalog : std::string) bool
}
FunctionMetadata ..> Util : uses getFunctionNameParts
note for Util "Previously enforced parts.size() == 3 with a VELOX_USER_CHECK; this constraint has been removed so it returns whatever parts are present."
note for FunctionMetadata "Now checks parts.size() < 3 and skips such functions instead of relying on Util to enforce a 3-part name."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- By removing the size check in getFunctionNameParts and only checking parts.size() < 3 in getFunctionsMetadata, inputs like an empty string or ".." will now produce parts with empty strings that are not filtered out, so consider adding validation or an explicit guard against empty segments before using schema/name indices.
- getFunctionsMetadata now repeats the same
parts.size() < 3 || skipCatalog(parts[0])check in three places; consider extracting this into a small helper or lambda to keep the logic consistent and easier to maintain if the criteria change again.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By removing the size check in getFunctionNameParts and only checking parts.size() < 3 in getFunctionsMetadata, inputs like an empty string or ".." will now produce parts with empty strings that are not filtered out, so consider adding validation or an explicit guard against empty segments before using schema/name indices.
- getFunctionsMetadata now repeats the same `parts.size() < 3 || skipCatalog(parts[0])` check in three places; consider extracting this into a small helper or lambda to keep the logic consistent and easier to maintain if the criteria change again.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
pramodsatya
left a comment
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 @PingLiuPing.
|
|
||
| const auto parts = util::getFunctionNameParts(name); | ||
| if (skipCatalog(parts[0])) { | ||
| if (parts.size() < 3 || skipCatalog(parts[0])) { |
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.
A helper function, skipFunction(std::vector<std::string>& parts), could be added in FunctionMetadata.cpp to include both these criteria?
Description
There is a force restriction on Velox function name such as
catalog.schema.function_name. If the function name does consist of three parts it leads to failure check immediately.This can causes worker node to crash etc. See discussion from #26584.
But we should not impose this rules from Prestissimo to Velox functions. Velox might register some functions that does not follow the Prestissimo rules. From Prestissimo point of view, it can just ignore those functions that do not have 3 part name.
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.