[Python] `bool(pa.scalar(False))` evaluates to True

[randolf-scholz]

Describe the bug, including details regarding any error messages, version, and platform.

This can easily cause bugs: assert pa.scalar(False) will not raise AssertionError!

Component(s)

Python

[zfoobar]

False is a valid value for a Boolean Scalar in this context, so it won't return 0, it will return an object specialized as boolean.

I guess the question would be: what wouldn't be acceptable to pyarrow as a scalar? Most of the types return scalar objects - including list, tuple, strings, etc.

[randolf-scholz]

@zfoobar pyarrow.BooleanScalar needs to implement the __bool__ magic method and return the appropriate python boolean. For example, here is how numpy does it:

b = np.bool_(False)
print(type(b))  # <class 'numpy.bool_'>
c = bool(b)
print(type(c))  # <class 'bool'>

[randolf-scholz]

I guess the question would be: what wouldn't be acceptable to pyarrow as a scalar? Most of the types return scalar objects - including list, tuple, strings, etc.

It's not about what is acceptable as a scalar. It's about which scalars should be considered Truthy and Falsey. In python the convention is:

• Boolean/Integer/Float/Complex: 0 → False, other →True
• Container-Types (string/list/tuple): same as bool(len(obj))

For other types (dates, timestamps, etc.) I'm not sure a boolean conversion makes sense.

[zfoobar]

Makes total sense.

[randolf-scholz]

I think it is worth discussing what to do in case when the boolean is null. There are 4 options:

1. Raise an Exception. For example, pandas raises a TypeError

import pandas as pd
bool(pd.NA)  # TypeError: boolean value of NA is ambiguous

2. Return None, which would evaluate to False in boolean context.
3. Coerce missing values to False
4. Coerce missing values to True

Options 3 and 4 do not make sense imo. Option 2 would have been my presonally preferred one if python was using None as part of a 3-valued logic. But it doesn't, so I think option 1 is currently the best bet.

In cases where the user does not want this exception, the as_py method could be given an additional keyword argument that handles the treatment of null.

[zfoobar]

@randolf-scholz PR updated with your wonderful insights. any additional thoughts upon review?

[randolf-scholz]

@zfoobar LGTM. Possibly pylint will complain about is the unnecessary else, but that's nitpicking.

[jorisvandenbossche]

While I certainly agree the current behaviour is not ideal, I am a bit hesitant to just add __bool__ to the boolean scalars.

• Several broader questions for __bool__ itself:
  • If we add it to BooleanScalar, we should probably add it to all other scalars as well?
  • If we add it to other scalar types, what should be its behaviour? For other numeric types it probably makes sense to follow 0 being falsey. But personally I am not fully convinced we should start making ListScalars truthy/falsey depending on the length of the list scalar value.
  • As @randolf-scholz already mentioned, we then also need to make a decision about the truthy value of null.
• But aside from bool(..) there are other aspects that people would then expect from normal "scalar" values, such as equality. Currently __eq__ is very dumb, so doing pa.scalar(None) == pa.scalar(None) gives True (while that doesn't match element-wise equality behavior for the array with equivalent values) or it requires exact types (again not being consistent with the element-wise "equal" kernel)

We could also say: if you want python-like scalar behaviour, just call as_py() on the pyarrow scalar.
(and we could also raise an error in __bool__ always, to make it clear this method isn't actually implemented)

[randolf-scholz]

@jorisvandenbossche

We could also say: if you want python-like scalar behaviour, just call as_py() on the pyarrow scalar.
(and we could also raise an error in bool always, to make it clear this method isn't actually implemented)

Certainly that could be better than the current situation. I opened this issue after the following code silently failed:

assert pa.compute.all(pa.compute.invert(array.is_null()))

Currently __eq__ is very dumb, so doing pa.scalar(None) == pa.scalar(None) gives True (while that doesn't match element-wise equality behavior for the array with equivalent values) or it requires exact types (again not being consistent with the element-wise "equal" kernel)

That just seems really bad behavior as well. pa.compute.equal(pa.scalar(None, type=pa.bool_()), pa.scalar(None, type=pa.bool_())) returns <pyarrow.BooleanScalar: None>, so pa.scalar(None, type=pa.bool_()) == pa.scalar(None, type=pa.bool_() should return this as well. This should be fine, since if the result of this equality will be used in context where a python boolean is expected the casting to bool will raise the TypeError, if we follow option 1 from my other comment.

[zfoobar]

Just coming at this from the lens of a newb user - not even considering the Null case:

>>> a = pa.scalar(1)
>>> b = pa.scalar(2)
>>> assert(pc.equal(a,b))
>>> 

^^ It wouldn't immediately occur to me to even know what as_py() is or to use it to ensure the expected behavior in this case. A solid RTFM could be in order but to my eye it wasn't immediately clear that this should be done.

Agree that __eq__ returning boolean object, which can then be relied on to provide __bool__ seems correct.

To @jorisvandenbossche's point, I did feel odd only adding __bool__ implementation for a single data type. =/

Happy to take another swing at this if we can align on an implementation, otherwise as_py() it is...