I think we can simplify this a bit:

and maybe something for the second iterator?

Thanks for the suggestion! I had indeed missed the existence of the keys method for simplifying this. Unfortunately I don't see any similar thing for directly iterating over the value fields inside the inner array, so I think that can't be simplified the same way.

could we have both test example validations? We would validate the previous functionality is still working as it was before maintaining the old test.

Sure, I restored the original test and tested this functionality in a new one.

What was the issue with the previous iterator?
I am unsure I understand what was wrong with the previous iteration and what are we fixing here.

Not the author of the MR, but the previous for key, value in self would result in a call of self.__iter__().

That thing is defined above this function, and does yield (k.as_py(), v.as_py()) directly. So it's hardcoded to the default maps_as_pydicts behaviour, which is incompatible to what we want here. The values, if they happen to be map types, basically would be yielded in the non-dict way (as the list of (key, value) tuples).

It's also not possible to adjust the __iter__() function because by definition it has to have no parameters, so it has to be opinionated in some sense about how to handle maps.

So in this case, we have to loop over the keys and values manually and then do the as_py() call on the value type with the correct maps_as_pydicts parameter ourselves.

I see, that makes sense, thanks for the clarification, should we validate self.values is not None as we currently do on __iter__?

        arr = self.values
        if arr is None:
            return
        for k, v in zip(arr.field(self.type.key_field.name), arr.field(self.type.item_field.name)):
            yield (k.as_py(), v.as_py())

I guess that would make sense, also adding a test for whether an empty map works could make sense? @jo-migo ?

Sure, I have added an explicit check for self.values is None and corresponding test.

As for the context, check out the PR description for a (hopefully) straightforward minimal example, but it's just like @jonded94 says. The problem with the current implementation is that round trips from python -> arrow -> python are broken at the moment for nested map columns ---

basically we want

{'x': {'a': {'1': 1}}} -> {'x': {'a': {'1': 1}}}

but the current behaviour with maps_as_pydicts is:

{'x': {'a': {'1': 1}}} -> {'x': {'a': [('1', 1)]}}