BUG: Fix map get with compatible types #1643
Conversation
ibis/expr/tests/test_value_exprs.py
Outdated
|
|
||
| def test_map_get_with_compatible_value_float(): | ||
| t = ibis.literal({'A': 1000, 'B': 2000}) | ||
| v = t.get('C', 3.0) |
There was a problem hiding this comment.
Hm, so this doesn't seem correct to me actually. Computing the precedence is too generic. We only want to be able to upcast within the same numeric kind not across types. non numeric types must be exactly equal.
There was a problem hiding this comment.
IMHO We should be strict in this case, I'd expect
t.get('C', 3.0) -> int(3)instead of3.0t.get('C', '3') -> int(3)instead of'3'
So default_type must be castable to value_type rather than using highest_precedence because it allows upcasting to both direction.
| # otherwise the result type is the value type since the user | ||
| # did not provide a default value | ||
| result_type = value_type | ||
| return rlz.shape_like(tuple(self.args), result_type) |
There was a problem hiding this comment.
How about
dtype = self.arg.type()
if not self.default.type().castable(dtype):
raise IbisTypeError(...)
return rlz.shape_like(self.args, dtype)?
There was a problem hiding this comment.
I think castable may still be too generic. I don't think that providing a float as the default for get on a map with integer value type should work. We may need some kind of map_get_compatible that is similar to castable, or maybe a strict_castable that is equivalent to castable but specializes on numeric types.
There was a problem hiding this comment.
We can introduce another flag into the overloaded castable functions
There was a problem hiding this comment.
Actually, it looks like castable does exactly what I want.
No description provided.