Fix 2-argument math functions

[harpaj]

Fixes the binary math functions:

• atan2 and hypot take two arguments, not one
• pow supports taking a literal numeric value as its second argument in addition to a Column.

[zero323]

Probably the best approach is to define the second argument as Union[Column, typing.SupportsFloat]

[harpaj]

Done! As seen in the code you linked above, this applies to both the first and the second argument though.

[zero323]

Done!

Thanks!

As seen in the code you linked above, this applies to both the first and the second argument though.

That's actually only partially true. While Python backend will accept (SupportsFloat, SupportsFloat) just fine, it will fail once call hits Py4j as JVM backend doesn't provide (Double, Double) => Column variant.

>>> from pyspark.sql.functions import atan2                                                                                                                                                                        
>>> atan2(3, 2)
Traceback (most recent call last):
...
Py4JError: An error occurred while calling z:org.apache.spark.sql.functions.atan2. Trace:
py4j.Py4JException: Method atan2([class java.lang.Double, class java.lang.Double]) does not exist
...

I guess the question is what we really want to support here... I guess these would be great:

• (Union[Column, str], Union[Column, str]) -> Column
• (SupportsFloat, Union[Column, str]) -> Column
• (Union[Column, str], SupportsFloat) -> Column

but I suspect (?) we might hit some unsafe overlaps at some point, as str <: SupportsFloat

[zero323]

Hi @harpaj. Do you plan to continue working on this?

[harpaj]

Hi, sorry about that, I was busy at the time you wrote and then forgot about it.
I made the changes you requested. Somehow the git history looks very weird locally for me - are you fine with just squashing on merge or should I try to clean it up?

[zero323]

Hi, sorry about that, I was busy at the time you wrote and then forgot about it.

No worries. I just wanted to know if I should take over the issue.

I made the changes you requested. Somehow the git history looks very weird locally for me - are you fine with just squashing on merge or should I try to clean it up?

Sure, that's not a big deal however, it looks like MyPy is unhappy with such formulation after all.

Traceback (most recent call last):
  File "/path/to/bin/mypy", line 10, in <module>
    sys.exit(console_entry())
....
RecursionError: maximum recursion depth exceeded while calling a Python object
third_party/3/pyspark/sql/functions.pyi:162: : note: use --pdb to drop into pdb

Interestingly enough this doesn't occur in isolation, i.e. such module

from typing import SupportsFloat, overload, Union                             
                                                                             
class Column: ...
                       
ColumnOrStr = Union[str, Column]
                                                                                
@overload                                                                       
def f(col1: ColumnOrStr, col2: ColumnOrStr) -> Column: ...
@overload
def f(col1: ColumnOrStr, col2: SupportsFloat) -> Column: ...
@overload
def f(col1: SupportsFloat, col2: ColumnOrStr) -> Column: ...

type checks just fine.

So it seems we're hitting some MyPy bug here, but I cannot say if it should fail, in a controlled way, in both cases, or rather pass both. I'll try to investigate this further, when I have some time to spare.

Though I guess it would be nice to wrap it up for now, especially because we're dealing with incorrect annotation.

I guess we can drop the protocol for now, i.e.

@overload
def hypot(col1: ColumnOrName, col2: ColumnOrName) -> Column: ...
@overload
def hypot(col1: float, col2: ColumnOrName) -> Column: ...
@overload
def hypot(col1: ColumnOrName, col2: float) -> Column: ...

It is a bit more restrictive than it suppose to, but it is almost as expressive as the other one. What do you think?

[harpaj]

What would you think of instead keeping the slightly more permission version of eeb854b?
I would tend to say that its better to be too permissive than too restrictive for type annotations.

[zero323]

To be honest I am not convinced. When in doubt I have strong preference towards false positive (type checker error, when the code is valid) over false negative (type checker pass, when code is invalid).

The rationale is simple here:

• If the code doesn't type check, user can always double check, and if necessary mark problematic code with type: ignore. This relatively cheap operation (nothing is executed yet), and can be applied even if type check is hard requirement (let's say in CI pipeline).
• If the code fails on runtime (false negative) then you might have already pay a lot of money, just to detect a simple mistake.

On a side I personally consider annotations as tool to hint what is intended (contract, over details of implementation) and what is good practice ("one-- and preferably only one --obvious way to do it").

Furthermore, due to Python's dynamism, it is often impossible to provide exhaustive annotations, so some false positives are simply to be expected.

[zero323]

I am closing this as superseded by #184 (combined history of this PR is included as fcf3a25 and ported to branch-2.3 and branch-2.4). Thanks for you contribution @harpaj!

[harpaj]

Sure, thanks a lot for all the time you put into this project!