Use DictionaryArray's iterator in compare_dict_op

[viirya]

Which issue does this PR close?

Closes #1329.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

[alamb]

The only potential concern I have here is that the take_iter()s use value() instead of value_unchecked() -- though perhaps we can merge this PR in and then add some additional unsafe / skipping bounds checking if it turns out to be in anyone's critical path / shows up in profiles

Thanks @viirya

[alamb]

This sure looks much nicer 👍

[viirya]

The only potential concern I have here is that the take_iter()s use value() instead of value_unchecked() -- though perhaps we can merge this PR in and then add some additional unsafe / skipping bounds checking if it turns out to be in anyone's critical path / shows up in profiles

Thanks @alamb . As we don't know if the keys in key iterator are fully in the valid range, seems we cannot skipping bounds check in take_iter? If it is critical, maybe we can have take_iter_unchecked in the future if it is necessary?

[alamb]

Thanks @alamb . As we don't know if the keys in key iterator are fully in the valid range,seems we cannot skipping bounds check in take_iter?

That is true in general. However, if we are iterating over a set of keys from a known valid DictionaryArray the indexes are known to be valid

If it is critical, maybe we can have take_iter_unchecked in the future if it is necessary?

I think this is wise strategy

[alamb]

I filed #1371 for follow on performance work and I am merging this one in now 🚀

thanks @viirya

[viirya]

Thank you @alamb !