[Python] StructScalar.as_py() fails if the type has duplicate field names

[asfimport]

StructScalar currently extends an abstract Mapping interface. Since the type allows duplicate field names we cannot provide that API.

Reporter: Krisztian Szucs / @kszucs

Related issues:

• [C++][Python] Refactor python to arrow conversions based on a reusable conversion API (relates to)

_{Note: This issue was originally created as ARROW-9997. Please see the migration documentation for further details.}

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
Personnally, I think moving away from the mapping API is quite a usability regression. Would it be a possibility to still provide that as the default, but have a alternative way to get a field by position?
(in a similar way, Table and Schema also work like that, they raise when getting a field by name and the name is duplicated, but you still can get a field by name)

[asfimport]

Krisztian Szucs / @kszucs:
I'd also prefer the mapping API, but .as_py() should not fail in any circumstance. If we keep the mapping API and return with a dict from .as_py(), then it is going to fail if there are duplicate field names.

If we add a mapping-like interface which tolerates duplicate keys end we implement it for both the StructScalar objects and for the one which is returned by StructScalar.as_py() then it could work.

cc @pitrou

[asfimport]

Antoine Pitrou / @pitrou:
Yes, I think a mapping-like is the answer.

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
This is marked being fixed by #8088, but could it be split off? (Since it is some code change in the cython StructScalar, it doesn't need to be tied to all other refactors?)

[asfimport]

Krisztian Szucs / @kszucs:
Definitely. I added it to have a proper roundtrip for the struct arrays, but I can skip that test.

[asfimport]

Neal Richardson / @nealrichardson:

I'd also prefer the mapping API, but .as_py() should not fail in any circumstance.

Is that so? If the mapping API is the most useful thing to translate a struct array into, and the only reason not to is the (uncommon) chance of duplicate field names, another solution would be to make as_py() error if there are duplicate field names. IIUC as_py is a convenience method, and since Python objects and the Arrow format are not identical, there should be no hard requirement that everything translate 100% faithfully. Put differently, usability and fidelity are tradeoffs at the extremes, and we should consider whether the benefit of increased fidelity in edge cases is worth the cost of usability in common cases.

[asfimport]

Krisztian Szucs / @kszucs:
My issue is StructScalar is an arrow object which implements a python mapping interface. Once we have duplicate keys the object stops to operate, we cannot do anything with it since all operation will raise a KeyError (not just when we call .as_py())

My another problem is that the struct array/scalar is the only type where we fail to roundtrip between arrow and python (at least according to a hypothesis test):

pa.array(arr.to_pylist(), type=arr.type)
pa.scalar(scalar.as_py(), type=scalar.type)

If we want convenient pythonic access to StructScalar I'd rather add a best effort .as_dict() method.

[asfimport]

Krisztian Szucs / @kszucs:
I find this issue a bit pressing before the release, but I'm not sure about the desired resolution. Perhaps returning with a multimap/multidict like object from scalar.as_py() would work (although it can cause backward incompatibilities if users expect dictionary instances).

@pitrou @jorisvandenbossche thoughts?

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
With multidict, you mean something like https://multidict.readthedocs.io/en/stable/multidict.html ?

My issue is StructScalar is an arrow object which implements a python mapping interface. Once we have duplicate keys the object stops to operate, we cannot do anything with it since all operation will raise a KeyError (not just when we call .as_py())

But that's something we could fix independenly from as_py(), I think?

For example, __contains__ relies on key access, which gives wrong results:

In [12]: s = pa.array([[('a', 1), ('b', 2), ('a', 3)]], pa.struct([('a', 'int64'), ('b', 'int64'), ('a', 'int64')]))[0]

In [13]: type(s)
Out[13]: pyarrow.lib.StructScalar

In [15]: list(s)
Out[15]: ['a', 'b', 'a']

In [16]: 'b' in s
Out[16]: True

In [17]: 'a' in s
Out[17]: False

But that's something we could fix if we want, without changing the default as_py behaviour.

And we could also add a method to get a field value by index instead of by name, which you could use if there are duplicate fields.

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
Given we didn't advance the discussion, moving to the next milestone

[asfimport]

Antoine Pitrou / @pitrou:
@amol- There is some contention here that needs to be resolved. It seems returning a mapping-like with some support for multiple values would be a possible solution. What is your opinion?

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
To restate from my last comment above, regardless of the as_py discussion, I think we can certainly already do:

• Fix the issue on the side of pyarrow's StructScalar, so that you can eg get a field value by index instead of by name using pyarrow's APIs + fix the cases where we have wrong behaviour (like the contains example). To ensure that at least our own StructScalar is usable and properly behaving with duplicate field names.

  That should already resolve one of the issues raised above. It doesn't resolve the discussion about the return type of as_py() of course.

  The return value of as_py as a dictionary is IMO the most useful one in a large majority of the use cases (and I don't think it "should not fail in any circumstance"). It's a reality that there is not always a direct 1:1 mapping with built-in python structures.

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
Small correction: you can already get a field by index by passing an integer to \_\_getitem\_\_:

In [19]: s = pa.scalar([('a', 1), ('b', 2), ('a', 3)], pa.struct([('a', 'int64'), ('b', 'int64'), ('a', 'int64')]))

In [20]: type(s)
Out[20]: pyarrow.lib.StructScalar

In [21]: s[0]
Out[21]: <pyarrow.Int64Scalar: 1>

The repr is also failing because that relies on as_py

[asfimport]

Joris Van den Bossche / @jorisvandenbossche:
I opened ARROW-13158 (and a PR) for some of the independent StructScalar fixes.