-
Notifications
You must be signed in to change notification settings - Fork 101
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
feat: Implementation of udf and udaf decorator #1040
base: main
Are you sure you want to change the base?
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.
This is a very nice addition and I love that you've already got some good unit tests.
I think from an end user perspective it might be slightly nicer if we could call these just @udf
instead of @udf_decorator
. I think it can be done.
This isn't my strongest suite, so I got a llm to generate this code:
import functools
class udf:
"""Acts both as a function and a decorator."""
def __new__(cls, func_or_value):
if callable(func_or_value):
return cls._decorator(func_or_value) # If used as a decorator
else:
return cls._function(func_or_value) # If used as a function
@staticmethod
def _function(value):
"""Original function behavior."""
return value * 2 # Example behavior
@staticmethod
def _decorator(func):
"""Decorator behavior."""
@functools.wraps(func)
def wrapper(*args, **kwargs):
print(f"Calling {func.__name__} with {args}, {kwargs}")
result = func(*args, **kwargs)
print(f"Result: {result}")
return result
return wrapper
Obviously we would need to adapt that some for our use case. What do you think?
Thanks for your suggestion! I totally agree that |
@timsaucer Hi, I borrowed your suggested idea and managed to get it work! I also used llm a bit to write the documentation. I hope it's not too confusing for users to understand that the APIs for function call and decorator call are slightly different (one with the callable parameter and one without the callable). Please let me know if you have any feedback on how to improve it! |
This is looking very nice. Would you mind if I do some wordsmithing on the documentation? I'll also run the work flows now. |
No problem! I will also fix the linting errors! |
Which issue does this PR close?
Closes #806
Rationale for this change
This PR implements decorators for
udf
andudaf
to make UDF creation more easily.Idea was suggested by: apache/datafusion-site#17 (comment)
What changes are included in this PR?
udf/udaf
methods.tests/test_udf.py
to make theis_null
test cases more reliable. Previously, since all data are not null, the return value is always[False, False, False]
, which is the same as the default empty vector. It caused confusion during my development because it didn't fail when I had a wrong implementation. I changed one value to NULL so that the output becomes[False, False, True]
, and it can test the functionality better.udf
to represent both a function and a decorator, we check if the first argument is aCallable
. If so, then it's a function all. If not, then it is a decorator call.Are there any user-facing changes?
Yes, this PR provides a more straightforward way for users to create UDF and UDAF.
Old way to create UDF:
New way to create UDF:
Old way to create UDAF:
New way to create UDAF: