Extended HMSET #130

aamalev · 2016-06-18T15:03:34Z

Implementation

coveralls · 2016-06-18T15:05:46Z

Coverage decreased (-0.002%) to 97.191% when pulling 27f974e on aamalev:master into 8726c2e on aio-libs:master.

coveralls · 2016-06-18T15:13:35Z

Coverage increased (+0.02%) to 97.21% when pulling 40f3c9b on aamalev:master into 8726c2e on aio-libs:master.

coveralls · 2016-06-19T12:01:21Z

Coverage increased (+0.001%) to 97.194% when pulling 6820357 on aamalev:master into 8726c2e on aio-libs:master.

popravich · 2016-06-21T13:21:18Z

Hi, this is good feature but I dont like your implementation -- it is too complex and has bugs.
First of all, if you add this test it will fail:

yield from redis.hmset('key', {'a': 1}, {'b': 2}, 'c', 3, d=4, a=5)
res = yield from redis.hgetall('key')
assert res == {b'a': b'5',
               b'b': b'2',
               b'c': b'3',
               b'd': b'4'}

Second, implementation, the public interface must be as follows:
hmset(key, *args, **kwargs)
internally this must be handled as follows:

check if not args and not kwargs -> TypeError
if len(args) == 1 -> args[0] must be a dict
or len(args) % 2 == 0 -> args is a field-value pairs
finally if len(args) != 1 or len(args) % 2 != 0 -> TypeError
add kwargs if any
and execute HMSET

aamalev · 2016-06-21T17:06:20Z

Hi @popravich. My implementation dicts_or_pairs not supported mixed types.

This test will fail in both cases in one and the same place.

def execute(self, command, *args, encoding=_NOTSET):
         ...
>       if None in set(args):
E       TypeError: unhashable type: 'dict'

A similar bug in hset:
yield from redis.hset('key', {'a': 1}, {'b': 2})

coveralls · 2016-06-21T17:15:28Z

Coverage increased (+0.02%) to 97.213% when pulling f77c6f8 on aamalev:master into 8726c2e on aio-libs:master.

coveralls · 2016-06-21T18:05:37Z

Coverage increased (+0.003%) to 97.196% when pulling 0135a20 on aamalev:master into 8726c2e on aio-libs:master.

popravich · 2016-06-21T19:49:41Z

A similar bug in hset:
yield from redis.hset('key', {'a': 1}, {'b': 2})

This is intended behaviour

My implementation dicts_or_pairs not supported mixed types.

If so it should raise exception when mixing arg types

coveralls · 2016-06-21T22:00:55Z

Coverage increased (+0.001%) to 97.194% when pulling 07134f9 on aamalev:master into 8726c2e on aio-libs:master.

coveralls · 2016-06-21T22:04:50Z

Coverage increased (+0.001%) to 97.194% when pulling 07134f9 on aamalev:master into 8726c2e on aio-libs:master.

aamalev · 2016-06-21T22:14:56Z

More successful implementation

coveralls · 2016-06-21T22:31:44Z

Coverage increased (+0.001%) to 97.194% when pulling 42e9e4e on aamalev:master into 8726c2e on aio-libs:master.

popravich · 2016-06-22T06:41:24Z

aioredis/commands/hash.py

+        if not args:
+            pairs = functools.reduce(operator.add, kwargs.items())
+
+        elif all(is_dict):


why do you need all args to be dicts?
what is the reason for calling hmset like this:

redis.hmset(key, {'a': 1}, {'b': 2})

and not simply with one dict?

aamalev · 2016-06-22T11:53:05Z

An example of the need OrderedDict

redis.hmset(key, {'a': 1}, {b'a': 2}, a=3)

As the regular dict:

{'a': 3, b'a':2} or {b'a': 2, 'a':3}

Another crazy example:

redis.hmset(key, {1: 1}, {b'1': 2}, {'1': 3})

coveralls · 2016-06-22T12:00:21Z

Coverage increased (+0.07%) to 97.262% when pulling 16314fb on aamalev:master into 8726c2e on aio-libs:master.

popravich · 2016-06-22T12:15:29Z

OMG this is totaly crazy)
It is end-user's responsibility to control input data.
Ok, what is gonna happen in this case (which value will win)?

redis.hmset(key, {b'a': 1, 'a': 2}, {'a': 3, b'a': 4})

kxepal · 2016-06-22T12:18:24Z

I agree with @popravich: no need to be too smart. If conflicts happens, better to cry loudly instead of picking decision that user may not be aware/expected about. Only user may solve them right.

popravich · 2016-06-22T12:25:31Z

I like it as simple as possible -- so it 100% clear how it works and what to expect.
Lets not break current hmset but create another one for dicts:
hmset_dict(key, *args, **kwargs) args allowed only to hold single a dict otherwise kwargs must be provided.
The implementation should simply unpack args[0] & kwargs into pairs and call original hmset

aamalev · 2016-06-22T15:02:50Z

Convinced. Therefore, I propose a simple implementation.

coveralls · 2016-06-22T15:03:04Z

Coverage increased (+0.02%) to 97.211% when pulling f2e9b57 on aamalev:master into 8726c2e on aio-libs:master.

kxepal · 2016-06-22T15:18:44Z

aioredis/util.py

+        pairs = args
+
+    if kwargs:
+        pairs = make_flat(kwargs.items(), pairs)


What's a point of generating tuples for every iteration? Sounds like you need in:

pairs = list(pairs) pairs.extend(chain.from_iterable(kwargs.items())

It's all said:

In [6]: %timeit make_flat(kwargs.items(), tuple()) 100 loops, best of 3: 2.45 ms per loop In [8]: %timeit list(chain.from_iterable(kwargs.items())) 10000 loops, best of 3: 78.2 µs per loop

Cool, but what is your kwargs?

In [46]: kwargs = dict(enumerate(string.ascii_lowercase)) In [47]: %timeit make_flat(kwargs.items(), tuple()) 100000 loops, best of 3: 11.1 µs per loop In [48]: %timeit make_flat(kwargs.items()) 100000 loops, best of 3: 10.3 µs per loop In [49]: %timeit list(chain.from_iterable(kwargs.items())) The slowest run took 5.11 times longer than the fastest. This could mean that an intermediate result is being cached. 100000 loops, best of 3: 6.89 µs per loop In [50]: %timeit list(chain.from_iterable(kwargs.items())) The slowest run took 5.22 times longer than the fastest. This could mean that an intermediate result is being cached. 100000 loops, best of 3: 6.93 µs per loop In [51]: %timeit tuple(chain.from_iterable(kwargs.items())) 100000 loops, best of 3: 6.04 µs per loop```

kwargs = {i:i for i in range(1000)}

coveralls · 2016-06-22T21:35:40Z

Coverage decreased (-1.4%) to 95.768% when pulling 5c00508 on aamalev:master into 8726c2e on aio-libs:master.

coveralls · 2016-06-22T21:35:40Z

Coverage decreased (-1.4%) to 95.768% when pulling 5c00508 on aamalev:master into 8726c2e on aio-libs:master.

coveralls · 2016-06-22T21:35:40Z

Coverage increased (+0.02%) to 97.21% when pulling 5c00508 on aamalev:master into 8726c2e on aio-libs:master.

popravich · 2016-06-23T07:01:22Z

thanks

popravich · 2016-06-23T07:56:01Z

PR is squashed and merged.
However I've split hmset into original and hmset_dict to decrease ambiguity of arguments' combinations.

aamalev · 2016-06-23T13:42:03Z

hmset_dict not the correct name, as well as the method of supporting the pair. Separate methods is that if someone thought to call the old hmset:

redis.hmset(key, field='a', value='1', 'b', '2', 'c', '3')

But I do not think that someone so does

aamalev · 2016-06-23T13:47:57Z

make_pair can also be used in mset

aamalev · 2016-06-23T13:54:15Z

Sorry, of course kwargs at the end

redis.hmset(key, 'b', '2', 'c', '3', field='a', value='1')

crazy?

popravich · 2016-06-23T13:56:52Z

redis.hmset(key, 'b', '2', 'c', '3', field='a', value='1')

This does not work -- multiple values for positional args

aamalev · 2016-06-23T14:04:32Z

I try to challenge the motivation of separation methods.
This work on the old method. But I think that no one uses this form

popravich · 2016-06-23T14:27:56Z

Ok, I want to keep original hmset as is because I don't want something that would fail in current version to success in newer.
I don't want old hmset to magically work if someone pass invalid args,
currently everyone using hmset must call it with field-value pairs positional arguments
and if someone try to call it with a dict -- will get TypeError.
Now imagine something like this:

hopefully_its_pairs = get_something()
yield from redis.hmset(key, *hopefully_its_pairs)

# and assume `get_something` is a function with lots of if/else:
def get_something():
    if case_a:
        return 'field', 'value'
    elif case_b:
        return 'field2', 'value2'
    elif ...
    elif ...
    elif case_N:
        return {'field': "value"}

Such cases may not be covered with tests and/or may appear only in production environment.
So if hmset had changed to allowed dicts this bug would suddenly disappear.
And it sounds more like a joke -- fixing bug in user's code by upgrading 3rd-party library.

aamalev · 2016-06-23T21:03:34Z

Reasonable.
I did not notice that hmset_dict only works with dict. For strictness propose to change the signature on

hmset_dict(key, fields: dict=None, **kwargs)

explicit arguments and simplified checks

popravich · 2016-06-24T06:56:17Z

this may be conflicting with user's fields:
hmset_dict(key, {'foo': 1, 'bar': 2}, fields='foo,bar')

aamalev · 2016-06-24T11:14:56Z

but current implementation already contains conflict

yield from redis.hmset_dict(key, key='kw')

aamalev added 2 commits June 18, 2016 17:49

Test extended HMSET

202441a

Extended HMSET

27f974e

Cover up

40f3c9b

Optimization HMSET

6820357

Change signature hmset

f77c6f8

hnset not supported mixed type of args

0135a20

aamalev added 2 commits June 22, 2016 00:58

HMSET: check args

f0dcdfb

HMSET: typo string

07134f9

HMSET: typo test

42e9e4e

popravich reviewed Jun 22, 2016
View reviewed changes

aamalev added 2 commits June 22, 2016 11:10

HMSET: test one dict

1a10df9

HMSET: effective one dict

1df3165

aamalev added 2 commits June 22, 2016 14:54

HMSET: add test

90a5546

HMSET refactoring: utils.make_pairs

16314fb

HMSET: simple

f2e9b57

kxepal reviewed Jun 22, 2016
View reviewed changes

HMSET: speed optimization

5c00508

popravich merged commit 2a12a16 into aio-libs-abandoned:master Jun 23, 2016

Extended HMSET #130

Extended HMSET #130

Conversation

aamalev commented Jun 18, 2016

coveralls commented Jun 18, 2016 • edited

coveralls commented Jun 18, 2016 • edited

coveralls commented Jun 19, 2016 • edited

popravich commented Jun 21, 2016

aamalev commented Jun 21, 2016

coveralls commented Jun 21, 2016 • edited

coveralls commented Jun 21, 2016 • edited

popravich commented Jun 21, 2016

coveralls commented Jun 21, 2016 • edited

coveralls commented Jun 21, 2016 • edited

aamalev commented Jun 21, 2016

coveralls commented Jun 21, 2016 • edited

popravich Jun 22, 2016

Choose a reason for hiding this comment

aamalev commented Jun 22, 2016 • edited

coveralls commented Jun 22, 2016 • edited

popravich commented Jun 22, 2016

kxepal commented Jun 22, 2016

popravich commented Jun 22, 2016

aamalev commented Jun 22, 2016

coveralls commented Jun 22, 2016 • edited

kxepal Jun 22, 2016

Choose a reason for hiding this comment

aamalev Jun 22, 2016

Choose a reason for hiding this comment

kxepal Jun 22, 2016

Choose a reason for hiding this comment

coveralls commented Jun 22, 2016

coveralls commented Jun 22, 2016

coveralls commented Jun 22, 2016 • edited

popravich commented Jun 23, 2016

popravich commented Jun 23, 2016

aamalev commented Jun 23, 2016 • edited

aamalev commented Jun 23, 2016

aamalev commented Jun 23, 2016

popravich commented Jun 23, 2016

aamalev commented Jun 23, 2016 • edited

popravich commented Jun 23, 2016

aamalev commented Jun 23, 2016

popravich commented Jun 24, 2016

aamalev commented Jun 24, 2016

coveralls commented Jun 18, 2016 •

edited

coveralls commented Jun 18, 2016 •

edited

coveralls commented Jun 19, 2016 •

edited

coveralls commented Jun 21, 2016 •

edited

coveralls commented Jun 21, 2016 •

edited

coveralls commented Jun 21, 2016 •

edited

coveralls commented Jun 21, 2016 •

edited

coveralls commented Jun 21, 2016 •

edited

aamalev commented Jun 22, 2016 •

edited

coveralls commented Jun 22, 2016 •

edited

coveralls commented Jun 22, 2016 •

edited

coveralls commented Jun 22, 2016 •

edited

aamalev commented Jun 23, 2016 •

edited

aamalev commented Jun 23, 2016 •

edited