-
Notifications
You must be signed in to change notification settings - Fork 5.5k
misc: Add function description in function metadata #26843
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 GuideAdds an optional additionalInformation field to FunctionMetadata and wires it through constructors and equality, then populates it from SQL-invoked functions in the JSON file-based function namespace manager using the function description. Sequence diagram for sqlInvokedFunctionToMetadata mapping additionalInformationsequenceDiagram
participant Manager as JsonFileBasedFunctionNamespaceManager
participant Func as SqlInvokedFunction
participant Sig as Signature
participant Params as List_Parameter
participant RC as RoutineCharacteristics
participant Meta as FunctionMetadata
Manager->>Func: getSignature()
Func-->>Manager: Sig
Manager->>Sig: getName()
Sig-->>Manager: QualifiedObjectName
Manager->>Sig: getArgumentTypes()
Sig-->>Manager: List_TypeSignature
Manager->>Func: getParameters()
Func-->>Manager: Params
Manager->>Params: map getName()
Params-->>Manager: List_String_argumentNames
Manager->>Sig: getReturnType()
Sig-->>Manager: TypeSignature
Manager->>Sig: getKind()
Sig-->>Manager: FunctionKind
Manager->>Func: getRoutineCharacteristics()
Func-->>Manager: RC
Manager->>RC: getLanguage()
RC-->>Manager: Language
Manager->>Manager: getFunctionImplementationType(function)
Manager-->>Manager: FunctionImplementationType
Manager->>Func: isDeterministic()
Func-->>Manager: boolean
Manager->>Func: isCalledOnNullInput()
Func-->>Manager: boolean
Manager->>Func: getVersion()
Func-->>Manager: FunctionVersion
Manager->>Func: getDescription()
Func-->>Manager: String_description
Manager->>Meta: new FunctionMetadata(name, argTypes, argNames, returnType, kind, language, implementationType, deterministic, calledOnNullInput, version, description)
Meta-->>Manager: FunctionMetadata_with_additionalInformation
Class diagram for updated FunctionMetadata and JSON namespace mappingclassDiagram
class FunctionMetadata {
- QualifiedObjectName name
- Optional~OperatorType~ operatorType
- List~TypeSignature~ argumentTypes
- Optional~List~ argumentNames
- TypeSignature returnType
- FunctionKind functionKind
- Optional~Language~ language
- FunctionImplementationType implementationType
- boolean deterministic
- boolean calledOnNullInput
- FunctionVersion version
- ComplexTypeFunctionDescriptor descriptor
- Optional~String~ additionalInformation
+ FunctionMetadata(name, argumentTypes, argumentNames, returnType, functionKind, language, implementationType, deterministic, calledOnNullInput)
+ FunctionMetadata(name, argumentTypes, argumentNames, returnType, functionKind, language, implementationType, deterministic, calledOnNullInput, version)
+ FunctionMetadata(name, argumentTypes, argumentNames, returnType, functionKind, language, implementationType, deterministic, calledOnNullInput, version, additionalInformation)
+ FunctionMetadata(name, argumentTypes, argumentNames, returnType, functionKind, language, implementationType, deterministic, calledOnNullInput, version, functionDescriptor)
+ FunctionMetadata(operatorType, argumentTypes, returnType, functionKind, implementationType, deterministic, calledOnNullInput, functionDescriptor)
+ Optional~String~ getAdditionalInformation()
+ boolean equals(obj)
+ int hashCode()
}
class JsonFileBasedFunctionNamespaceManager {
# Collection~SqlInvokedFunction~ fetchFunctionsDirect(name)
# FunctionMetadata sqlInvokedFunctionToMetadata(function)
}
class SqlInvokedFunction {
+ Signature getSignature()
+ List~Parameter~ getParameters()
+ RoutineCharacteristics getRoutineCharacteristics()
+ boolean isDeterministic()
+ boolean isCalledOnNullInput()
+ FunctionVersion getVersion()
+ String getDescription()
}
class Signature {
+ QualifiedObjectName getName()
+ List~TypeSignature~ getArgumentTypes()
+ TypeSignature getReturnType()
+ FunctionKind getKind()
}
class Parameter {
+ String getName()
}
class RoutineCharacteristics {
+ Language getLanguage()
}
JsonFileBasedFunctionNamespaceManager ..> SqlInvokedFunction : uses
JsonFileBasedFunctionNamespaceManager ..> FunctionMetadata : creates
SqlInvokedFunction --> Signature : has
SqlInvokedFunction --> Parameter : has many
SqlInvokedFunction --> RoutineCharacteristics : has
FunctionMetadata --> ComplexTypeFunctionDescriptor : has
FunctionMetadata --> FunctionVersion : has
FunctionMetadata --> Language : optional
FunctionMetadata --> OperatorType : optional
FunctionMetadata --> FunctionKind : has
FunctionMetadata --> TypeSignature : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary: Pull Request resolved: prestodb#26843 Differential Revision: D89610313
cf60222 to
52206b6
Compare
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 - I've found 1 issue, and left some high level feedback:
- In the new
FunctionMetadataconstructor that accepts aString additionalInformation, you wrap it withOptional.of(additionalInformation); iffunction.getDescription()can be null (as is common for descriptions), this will NPE, so either enforce non-null at the caller or switch to acceptingOptional<String>and usingOptional.ofNullable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new `FunctionMetadata` constructor that accepts a `String additionalInformation`, you wrap it with `Optional.of(additionalInformation)`; if `function.getDescription()` can be null (as is common for descriptions), this will NPE, so either enforce non-null at the caller or switch to accepting `Optional<String>` and using `Optional.ofNullable`.
## Individual Comments
### Comment 1
<location> `presto-function-namespace-managers/src/main/java/com/facebook/presto/functionNamespace/json/JsonFileBasedFunctionNamespaceManager.java:188` </location>
<code_context>
+ function.isDeterministic(),
+ function.isCalledOnNullInput(),
+ function.getVersion(),
+ function.getDescription());
+ }
+
</code_context>
<issue_to_address>
**question (bug_risk):** Clarify/ensure non-nullability of `function.getDescription()` when passing to the new `FunctionMetadata` constructor
Because the new `FunctionMetadata` overload wraps this in `Optional.of(...)`, any null from `SqlInvokedFunction.getDescription()` will cause an NPE. If `description` can be null/omitted, normalize it here (e.g., convert null to `Optional.empty()`) or change the constructor to accept an `Optional<String>` to match the internal representation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...n/java/com/facebook/presto/functionNamespace/json/JsonFileBasedFunctionNamespaceManager.java
Show resolved
Hide resolved
Summary: Pull Request resolved: prestodb#26843 Differential Revision: D89610313
52206b6 to
311a6a1
Compare
Description
Add function description in function metadata, and populate this field in the Json function manager
Motivation and Context
Add function description in function metadata
Impact
Add function description in function metadata
Test Plan
Simple change, existing unit tests
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.