ARROW-2515 [Python] Add DictionaryValue class, fixing bugs with nested dictionaries

[blkerby]

This introduces a scalar value class DictionaryValue, which fixes a couple bugs involving dictionaries nested inside of ListArrays or inside of other DictionaryArrays. This also includes a new test, which failed previous to this commit but now passes. See https://issues.apache.org/jira/browse/ARROW-2515.

This is my first time contributing, so feedback would be most welcome.

[pitrou]

It's a single index, so how about calling this index?

[blkerby]

My thinking was to try to be consistent with the naming of the indices and dictionary properties of the DictionaryArray class, but I agree it's unwieldy and should be changed to simpler names if possible. My first thought was to go with just index and value as well, but is there a potential clash with the index member of the parent class ArrayValue? I guess right now the inherited index is not visible (since it was declared in the Cython and not exposed as a property), but any chance this will change? It seems like all three of these could potentially have some use: index being the index into the indices array, indices_value the value in the indices array (which means it is also the index into the dictionary array), and then dictionary_value being the value in the dictionary array.

But then again I guess it's possible to think of the ArrayValue index field as an implementation detail which maybe should never be exposed in the API, since it doesn't really provide information about the value itself but rather about its origin. If it is ever exposed, I suppose it could be through a different name, maybe something deliberately unappealing, such as _index, and in that case there's no name clash to be worried about in introducing index and value as DictionaryValue properties.

[pitrou]

Oh, yes, that's a good point. Then I would suggest:

• keep the current index for the index into the indices array
• use index_value for the value in the indices array
• keep dictionary_value for the value in the dictionary array

[pitrou]

Perhaps simply value?

Edit: Actually, no, see above :-)

[pitrou]

Thanks for your contribution @blkerby ! In addition to the naming nits I posted, it would be nice to add a test for the new DictionaryValue properties in test_scalars.py. Could you do that?

[blkerby]

Excellent suggestion, I'd be happy to add tests for those!

[blkerby]

Done!

[pitrou]

LGTM now.

[pitrou]

Thanks for your contribution @blkerby !