-
Notifications
You must be signed in to change notification settings - Fork 5.5k
misc: Add lookup cast in standard function resolution #26657
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
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR adds a dedicated lookupCast API to the SQL function resolution layer, enabling explicit resolution of cast functions by input and output types, and updates unit tests to verify its behavior. Class diagram for updated StandardFunctionResolution interface and FunctionResolution implementationclassDiagram
class StandardFunctionResolution {
+boolean isCastFunction(FunctionHandle functionHandle)
+FunctionHandle lookupCast(String castType, Type fromType, Type toType)
+boolean isCountFunction(FunctionHandle functionHandle)
+boolean isCountIfFunction(FunctionHandle functionHandle)
}
class FunctionResolution {
+boolean isCastFunction(FunctionHandle functionHandle)
+FunctionHandle lookupCast(String castType, Type fromType, Type toType)
+boolean isTryCastFunction(FunctionHandle functionHandle)
}
StandardFunctionResolution <|.. FunctionResolution
FunctionResolution --> functionAndTypeResolver: uses
functionAndTypeResolver : lookupCast(castType, fromType, toType)
functionAndTypeResolver : getFunctionMetadata(functionHandle)
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:
- The new SPI method lookupCast should either have a default implementation or be added to all existing implementations to avoid breaking backwards compatibility.
- Rather than passing a raw string for the cast function name, consider using a QualifiedObjectName or enum to enforce type safety and prevent typos in the lookupCast API.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new SPI method lookupCast should either have a default implementation or be added to all existing implementations to avoid breaking backwards compatibility.
- Rather than passing a raw string for the cast function name, consider using a QualifiedObjectName or enum to enforce type safety and prevent typos in the lookupCast API.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
hantangwangd
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.
LGTM!
Description
cast function is special as it needs to specify both input and output types, hence cannot use the existing lookupBuiltInFunction API. Add the lookupCast API in this PR
Motivation and Context
See description
Impact
See description
Test Plan
Unit test
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.