feat(typing): make types more precise for many common Values#11771
feat(typing): make types more precise for many common Values#11771cpcloud merged 1 commit intoibis-project:mainfrom
Conversation
cpcloud
left a comment
There was a problem hiding this comment.
Unless there's something I'm missing here I don't see how return-type-only overloads are actually useful.
ibis/expr/types/generic.py
Outdated
| return ibis.dense_rank().over(order_by=self) | ||
|
|
||
| def percent_rank(self) -> Column: | ||
| def percent_rank(self) -> ir.IntegerColumn: |
There was a problem hiding this comment.
I don't think this guaranteed to be an IntegerColumn. Consider the example in the docstring.
There was a problem hiding this comment.
Thanks you're right
There was a problem hiding this comment.
Adjusted to ir.FloatingColumn
ibis/expr/types/logical.py
Outdated
| self: ir.BooleanScalar, true_expr: ir.Scalar, false_expr: ir.Scalar, / | ||
| ) -> ir.Scalar: ... | ||
| @overload | ||
| def ifelse(self, true_expr: ir.Value, false_expr: ir.Value, /) -> ir.Column: ... |
There was a problem hiding this comment.
Why do we need overloads for these? You can't use overload for return values anyway. Consider this example:
@overload
def foo(x: int) -> str: ...
@overload
def foo(x: int) -> int: ...
y = foo(1) # which overload is called?There's no straightforward way to compute the type of y.
There was a problem hiding this comment.
What I was going for was more of
@overload
def foo(x: SubtypeOfInt) -> str: ...
@overload
def foo(x: int) -> int: ...so that the first overload would be selected as the first matching case. I can do some more tests though to ensure that this is actually happening.
|
@NickCrews Can't you split the changes that make return types more precise out into a separate PR that doesn't contain the changes with all the additional overloads? |
|
Yes! |
3288afd to
d38ac20
Compare
|
OK in that new version I removed all overloads, but I did go through even more methods and made them more correct and accepting of plain python values and Deferreds, eg adjusting |
d38ac20 to
685d725
Compare
685d725 to
0f9c078
Compare
This partially mitigates #11680, at least for some of the more common types.
In particular, it solves this ask:
#11680 (comment)
I focused on the most common use cases, eg
StringValue + "foo"is more important than NumericColumn.kurtosis()This fixes several sorts of problems:
@overloadbecause the method depends on the datashape of second args, or it is an eg string->int methodStringValue.__getitem__, it actually accepts IntegerColumn as well, not just IntegerScalar, as it was typed before. Or IntegerValue.convert_base() incorrectly said it returned a IntegerValue, but it actually returns a StringValue