ibis icon indicating copy to clipboard operation
ibis copied to clipboard

feat(ir): cast operands of `Comparable` to be the same type

Open timothydijamco opened this issue 3 years ago • 3 comments

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:

  1. (3.0.0) Comparable checks 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.
  2. Comparable checks if operands are castable to each other. The backends, in their execution/compilation functions for Comparable nodes, would have some extra logic to cast similar types to each other before comparing.
  3. (2.1.1) Comparable checks 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.

timothydijamco avatar Jul 27 '22 14:07 timothydijamco

Thanks @timothydijamco.

@gforsyth @saulpw Curious what y'all think here.

cpcloud avatar Jul 27 '22 14:07 cpcloud

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

  1. Fail, because the types are incompatible
  2. Succeed, because they are compatible AND the backend in question performs the cast for us
  3. 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.

gforsyth avatar Jul 27 '22 19:07 gforsyth

@timothydijamco Are you still interested in tackling this issue? I moved it off the 3.2.0 milestone for now.

cpcloud avatar Sep 12 '22 20:09 cpcloud