feat(ir): cast operands of `Comparable` to be the same type
In 2.1.1, Comparable would check if its operands can be cast to the same type, raises an error if not, then performs the cast. This was changed in 3.0.0 in https://github.com/ibis-project/ibis/commit/e929f85b4ffadad432e6d8ee7267c58aea062454. Now we check if they are castable, raise an error if not, but don't perform the cast.
Example
import datetime as dt
import ibis
t = ibis.table([('time', 'timestamp'), ('foo', 'float'), ('bar', 'int32')], 'my_data')
t = t.filter(t['time'] == dt.date(2022, 7, 1))
print(ibis.bigquery.compile(t))
2.1.1
SELECT *
FROM my_data
WHERE `time` = TIMESTAMP '2022-07-01'
3.0.0
SELECT *
FROM my_data
WHERE `time` = DATE '2022-07-01'
I initially thought this might be a regression as it seemed like an incidental change in that commit, but I'm not sure if it was and the new behavior isn't necessarily incorrect. I wanted to clarify/discuss what the desired behavior for this is. I see three options:
- (3.0.0)
Comparablechecks if operands are castable to each other, as a rough validation check of "comparability". The operands are not cast to each other at the Ibis expression level. The backends handle the comparisons in their native way (specifically, in our backends I don't think we have any extra logic in the backends that casts similar types to each other before comparing). Users should explicitly cast operands if they want. -
Comparablechecks if operands are castable to each other. The backends, in their execution/compilation functions forComparablenodes, would have some extra logic to cast similar types to each other before comparing. - (2.1.1)
Comparablechecks if operands are castable to each other, then casts them at the Ibis expression level.
I think casting operands to be the same type is helpful for UX and unifying behavior across backends, but I can see the argument for asking users to be more specific/explicit.
Thanks @timothydijamco.
@gforsyth @saulpw Curious what y'all think here.
I'm in favor of doing the cast here as a part of the comparison.
As it stands now (to my reading), if I perform a comparison, it can
- Fail, because the types are incompatible
- Succeed, because they are compatible AND the backend in question performs the cast for us
- Succeed in creating the expression, but fail on execution, because they are compatible but the backend doesn't cast for us.
That seems like it could be friendlier.
@timothydijamco Are you still interested in tackling this issue? I moved it off the 3.2.0 milestone for now.