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

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/source/release.rst
Expand Up @@ -34,6 +34,7 @@ New Features
* ``ibis.pandas.from_dataframe`` convenience function (:issue:`1155`)
* Remove the restriction on ``ROW_NUMBER()`` requiring it to have an
``ORDER BY`` clause (:issue:`1371`)
* Add ``.get()`` operation on a Map type (:issue:`1376`)

Bug Fixes
~~~~~~~~~
Expand Down
14 changes: 14 additions & 0 deletions ibis/expr/api.py
Expand Up @@ -1992,13 +1992,27 @@ def _array_slice(array, index):

_add_methods(ArrayValue, _array_column_methods)


# ---------------------------------------------------------------------
# Map API

def get(expr, key, default):
"""
Return the mapped value for this key, or the default
if the key does not exist

Parameters
----------
key : any
default : any
"""
return _ops.MapValueOrDefaultForKey(expr, key, default).to_expr()


_map_column_methods = dict(
length=_unary_op('length', _ops.MapLength),
__getitem__=_binop_expr('__getitem__', _ops.MapValueForKey),
get=get,
keys=_unary_op('keys', _ops.MapKeys),
values=_unary_op('values', _ops.MapValues),
__add__=_binop_expr('__add__', _ops.MapConcat),
Expand Down
21 changes: 21 additions & 0 deletions ibis/expr/operations.py
Expand Up @@ -2746,6 +2746,27 @@ def output_type(self):
return rules.shape_like(self.args[0], map_type.value_type)


class MapValueOrDefaultForKey(ValueOp):

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

]

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


if not (default_type is dt.null or
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the default value for default be NULL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh whoops, nevermind. This is correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep.

value_type.equals(default_type)):
raise ValueError("default type: {} must be the same "
"as the map value_type {}".format(
default_type, value_type))
return rules.shape_like(self.args[0], map_type.value_type)
Copy link
Member

@cpcloud cpcloud Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



class MapKeys(ValueOp):

input_type = [rules.map(dt.any, dt.any)]
Expand Down
16 changes: 13 additions & 3 deletions ibis/expr/tests/test_value_exprs.py
Expand Up @@ -150,13 +150,23 @@ def test_simple_map_operations():
expr = ibis.literal(value)
expr2 = ibis.literal(value2)
assert isinstance(expr, ir.MapValue)
assert isinstance(expr['b'].op(), ops.MapValueForKey)
assert isinstance(expr.length().op(), ops.MapLength)
assert isinstance(expr.keys().op(), ops.MapKeys)
assert isinstance(expr.values().op(), ops.MapValues)
assert isinstance((expr + expr2).op(), ops.MapConcat)
assert isinstance((expr2 + expr).op(), ops.MapConcat)

default = ibis.literal([0.0])
assert isinstance(expr.get('d', default).op(), ops.MapValueOrDefaultForKey)

# test for an invalid default type, nulls are ok
with pytest.raises(ValueError):
expr.get('d', ibis.literal('foo'))
assert isinstance(expr.get('d', ibis.literal(None)).op(),
ops.MapValueOrDefaultForKey)

assert isinstance(expr['b'].op(), ops.MapValueForKey)
assert isinstance(expr.keys().op(), ops.MapKeys)
assert isinstance(expr.values().op(), ops.MapValues)


@pytest.mark.parametrize(
['value', 'expected_type'],
Expand Down