-
Notifications
You must be signed in to change notification settings - Fork 113
fix: type checking #993
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
fix: type checking #993
Changes from all commits
124d3ea
a933fdb
9897b63
768b6c0
3250848
b165ef3
2c8ea03
a7993a1
0d6cd07
d771d3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -85,7 +85,7 @@ class ScalarUDF: | |||
|
||||
def __init__( | ||||
self, | ||||
name: Optional[str], | ||||
name: str, | ||||
func: Callable[..., _R], | ||||
input_types: pyarrow.DataType | list[pyarrow.DataType], | ||||
return_type: _R, | ||||
|
@@ -182,7 +182,7 @@ class AggregateUDF: | |||
|
||||
def __init__( | ||||
self, | ||||
name: Optional[str], | ||||
name: str, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why remove the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it seems that in rust binding, it's not optional Line 92 in e6f6e66
|
||||
accumulator: Callable[[], Accumulator], | ||||
input_types: list[pyarrow.DataType], | ||||
return_type: pyarrow.DataType, | ||||
|
@@ -277,6 +277,7 @@ def sum_bias_10() -> Summarize: | |||
) | ||||
if name is None: | ||||
name = accum.__call__().__class__.__qualname__.lower() | ||||
assert name is not None | ||||
if isinstance(input_types, pyarrow.DataType): | ||||
input_types = [input_types] | ||||
return AggregateUDF( | ||||
|
@@ -462,7 +463,7 @@ class WindowUDF: | |||
|
||||
def __init__( | ||||
self, | ||||
name: Optional[str], | ||||
name: str, | ||||
func: Callable[[], WindowEvaluator], | ||||
input_types: list[pyarrow.DataType], | ||||
return_type: pyarrow.DataType, | ||||
|
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 changing the user-facing API, no?
Uh oh!
There was an error while loading. Please reload this page.
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.
i checked the test,i think we should use @ property, but previously we didnt do wrapping somewhere, raw table is returned that's why test passed.
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.
I've become convinced that the current code is broken and that this is the right change. Thank you @chenkovsky for doing this. I do think we need to put some notes to that effect in the section of user facing changes of the PR description.