Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Fix map get with compatible types #1643

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
2 participants
@cpcloud
Copy link
Member

commented Oct 1, 2018

No description provided.

@cpcloud cpcloud force-pushed the cpcloud:map-get-bug branch from 0d3e536 to b21b919 Oct 1, 2018

@cpcloud cpcloud added this to the Next Release milestone Oct 1, 2018

@cpcloud cpcloud requested a review from kszucs Oct 1, 2018

@cpcloud cpcloud added the expressions label Oct 1, 2018


def test_map_get_with_compatible_value_float():
t = ibis.literal({'A': 1000, 'B': 2000})
v = t.get('C', 3.0)

This comment has been minimized.

Copy link
@cpcloud

cpcloud Oct 1, 2018

Author Member

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.

This comment has been minimized.

Copy link
@kszucs

kszucs Oct 10, 2018

Member

IMHO We should be strict in this case, I'd expect

  • t.get('C', 3.0) -> int(3) instead of 3.0
  • t.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.

This comment has been minimized.

Copy link
@kszucs

kszucs Oct 10, 2018

Member

See my comment below.

# 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)

This comment has been minimized.

Copy link
@kszucs

kszucs Oct 10, 2018

Member

How about

dtype = self.arg.type()

if not self.default.type().castable(dtype):
    raise IbisTypeError(...)

return rlz.shape_like(self.args, dtype)

?

This comment has been minimized.

Copy link
@cpcloud

cpcloud Oct 11, 2018

Author Member

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.

This comment has been minimized.

Copy link
@kszucs

kszucs Oct 12, 2018

Member

We can introduce another flag into the overloaded castable functions

This comment has been minimized.

Copy link
@cpcloud

cpcloud Oct 20, 2018

Author Member

Actually, it looks like castable does exactly what I want.

@cpcloud cpcloud force-pushed the cpcloud:map-get-bug branch from b21b919 to 775c703 Oct 19, 2018

@cpcloud cpcloud force-pushed the cpcloud:map-get-bug branch from 775c703 to 51feb03 Oct 20, 2018

cpcloud added some commits Oct 23, 2018

@cpcloud cpcloud closed this in ec13ef6 Oct 28, 2018

@cpcloud cpcloud deleted the cpcloud:map-get-bug branch Oct 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.