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

API: add .get() operation on a Map type #1376

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jreback
Contributor

jreback commented Mar 5, 2018

No description provided.

@jreback jreback added the enhancement label Mar 5, 2018

@jreback jreback added this to the 0.13 milestone Mar 5, 2018

input_type = [
rules.map(dt.any, dt.any),
rules.one_of((dt.string, dt.int_), name='key'),
rules.value(name='default')

This comment has been minimized.

@cpcloud

cpcloud Mar 7, 2018

Member

This really should be the concrete type of the values. Not sure if we can figure this out when we validate this. I think we could, but I don't think there's a rules API for dealing with this.

This comment has been minimized.

@jreback

jreback Mar 7, 2018

Contributor

this should just be dt.any, but .value doesn't like a type specified

This comment has been minimized.

@cpcloud

cpcloud Mar 7, 2018

Member

This is fine, but see my comment below about output_type.

def output_type(self):
map_type = self.args[0].type()
return rules.shape_like(self.args[0], map_type.value_type)

This comment has been minimized.

@cpcloud

cpcloud Mar 7, 2018

Member

You should check that the type of default is null or equivalent to map value type.

@cpcloud

This comment has been minimized.

Member

cpcloud commented Mar 7, 2018

@jreback Can you implement this on at least one backend and add a test? Otherwise, LGTM.

@jreback

This comment has been minimized.

Contributor

jreback commented Mar 7, 2018

we don't have any map operations implemented on any backends, they are simply abstract at this point. what were you thinking of?

@cpcloud

This comment has been minimized.

Member

cpcloud commented Mar 7, 2018

@jreback hm, i thought we had some on the pandas backend, guess I was mistaken. You can ignore my comment. This LGTM. merging shortly.

@cpcloud

This comment has been minimized.

Member

cpcloud commented Mar 7, 2018

Acutally, still have the comment about output_type. Will merge after that is addressed.

@jreback

This comment has been minimized.

Contributor

jreback commented Mar 7, 2018

we could make the Map operations work on a Series actually.

@jreback

This comment has been minimized.

Contributor

jreback commented Mar 7, 2018

fixed up. I think my way of checking null (type) is idiomatic.

def output_type(self):
map_type = self.args[0].type()
value_type = map_type.value_type
default_type = self.args[2].type()

This comment has been minimized.

@cpcloud

cpcloud Mar 7, 2018

Member

you can do self.default.type() here which is a bit more readable.

This comment has been minimized.

@jreback

jreback Mar 7, 2018

Contributor

done

value_type = map_type.value_type
default_type = self.args[2].type()
if not (default_type is dt.null or

This comment has been minimized.

@cpcloud

cpcloud Mar 7, 2018

Member

Shouldn't the default value for default be NULL

This comment has been minimized.

@jreback

jreback Mar 7, 2018

Contributor

good point

This comment has been minimized.

@jreback

jreback Mar 7, 2018

Contributor

pushed, I don't know how to make default optional though, rules.value doesn't like this

value_type = map_type.value_type
default_type = self.default.type()
if not (default_type is dt.null or

This comment has been minimized.

@cpcloud

cpcloud Mar 7, 2018

Member

It's okay for this to be null since that would be the default return value if a key doesn't exist and default is not provided.

This comment has been minimized.

@cpcloud

cpcloud Mar 7, 2018

Member

Oh whoops, nevermind. This is correct.

This comment has been minimized.

@cpcloud

cpcloud Mar 7, 2018

Member

Null checking looks good here.

Small nit: can you write this as if default_type is not dt.null and value_type != default_type. It's a little easier to read IMO.

Then will merge.

This comment has been minimized.

@jreback

jreback Mar 7, 2018

Contributor

yep.

@jreback

This comment has been minimized.

Contributor

jreback commented Mar 7, 2018

pushed

@cpcloud

cpcloud approved these changes Mar 7, 2018

LGTM, merging on green

@cpcloud cpcloud closed this in 1127162 Mar 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment