-
Notifications
You must be signed in to change notification settings - Fork 10
Add context to table UDFs #56
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
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.
Pull Request Overview
This PR adds context support to table UDFs by introducing a new BindArgumentsContext field to the tableFunction struct. This allows table UDFs to retrieve values from the query context, similar to how scalar UDFs already have this capability.
- Adds
BindArgumentsContextfield totableFunctionstruct for context-aware binding - Updates wrapper functions to handle both
BindArgumentsandBindArgumentsContext - Modifies bind logic to prioritize
BindArgumentsContextwhen available, falling back toBindArguments - Introduces
tableFuncContextstruct to pass context store to bind functions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| table_udf.go | Adds BindArgumentsContext field to tableFunction, updates wrappers to support both bind methods, extracts and loads context from connection during binding |
| table_udf_test.go | Adds contextTableUDF test implementation and TestContextTableUDF to verify context values can be passed to table UDFs |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Out of curiosity - is the copilot review something you enabled in your settings? I.e., I can't seem to find anything related to that on the repository level and AFAIK I did not enable it. 🤔 |
|
Yes, it's the private copilot, I also have it. |
|
Didn't realize it would trigger on other people's repos, that's interesting. But yes, I have a subscription so that's why it got triggered. Any feedback on this version of the change? I think it's pretty straightforward and in line with the scalar UDF one |
|
Code looks good - could you have a look at the failing tests? |
|
@wmTJc9IK0Q Looks really good! |
|
@taniabogatsch Fixed. Thanks. |
|
Thanks! |
This adds a new BindArgumentsContext field to the
tableFunctionstruct to allow table UDFs to retrieve values from the query context.This was easier to make as a non-breaking change unlike the previous #53 and doesn't change the ability to use this context during the Fill* funcs if necessary.