ARROW-8426: [Rust] [Parquet] - Add more support for converting Dicts

[carols10cents]

This adds more support for:

• When converting Arrow -> Parquet containing an Arrow Dictionary,
  materialize the Dictionary values and send to Parquet to be encoded with
  a dictionary or not according to the Parquet settings (deliberately not supporting
  converting an Arrow Dictionary directly to Parquet DictEncoding, and right now this only supports String dictionaries)
• When converting Parquet -> Arrow, noticing that the Arrow schema
  metadata in a Parquet file has a Dictionary type and converting the data
  to an Arrow dictionary (right now this only supports String dictionaries)

I'm not sure if this is in a good enough state to merge or not yet, please let me know @nevi-me !

[github-actions]

https://issues.apache.org/jira/browse/ARROW-8426

[nevi-me]

Parquet's dictionary encoding is a complexity on its own. My understanding's that after a certain size, the dictionary no longer grows, but the additional values are stored the normal way. I'm still to spend more time on parquet-mr and the format.
I think the approach of not forcing Arrow dictionaries to have Parquet dictionary encoding is good.

also only supports Int32 index types in this commit, also removes NULLs

Do you want to work on other index types and supporting primitive Arrow dictionaries? We could keep this PR open for longer; as long as it's not blocking any additional unit of work.

[carols10cents]

Do you want to work on other index types and supporting primitive Arrow dictionaries? We could keep this PR open for longer; as long as it's not blocking any additional unit of work.

Yup, I'm happy to do that! I'll be rebasing, addressing feedback, and adding to this on Wednesday.

[carols10cents]

Status update: The other index types are done, but primitive dictionaries are not yet.

[carols10cents]

@vertexclique @nevi-me I'm feeling stuck on converting primitive dictionaries...

I have a solution that works for one key/value type, but I tried to expand that to all the types and it involves listing out all the possible combinations (😱) and overflows the stack (😱😱😱).

I have tried to find a different abstraction, though, and the type checker doesn't like anything I've come up with. Do you have any suggestions?

[alamb]

@vertexclique @nevi-me I'm feeling stuck on converting primitive dictionaries...

I have a solution that works for one key/value type, but I tried to expand that to all the types and it involves listing out all the possible combinations (😱) and overflows the stack (😱😱😱).

I have tried to find a different abstraction, though, and the type checker doesn't like anything I've come up with. Do you have any suggestions?

@carols10cents -- one idea I had which might be less efficient at runtime but possibly be less complicated to implement, would be to use the arrow cast kernels here: https://github.com/apache/arrow/blob/master/rust/arrow/src/compute/kernels/cast.rs

So rather than going directly from ParquetType to DesiredArrowType we could go from ParquetType --> CanonicalArrowType and then from CanonicalArrowType --> DesiredArrowType

So for example, to generate a Dictionary<UInt8, Utf8> from a parquet column of Utf8 you could always create Dictionary<Uint64, Utf8> and then use cast to go to the desired arrow type

Does that make sense?

[carols10cents]

@carols10cents -- one idea I had which might be less efficient at runtime but possibly be less complicated to implement, would be to use the arrow cast kernels here: https://github.com/apache/arrow/blob/master/rust/arrow/src/compute/kernels/cast.rs

So rather than going directly from ParquetType to DesiredArrowType we could go from ParquetType --> CanonicalArrowType and then from CanonicalArrowType --> DesiredArrowType

So for example, to generate a Dictionary<UInt8, Utf8> from a parquet column of Utf8 you could always create Dictionary<Uint64, Utf8> and then use cast to go to the desired arrow type

Does that make sense?

Not really, because I am using the cast kernels in the Converter: 4b59fc9 (#8402) in the style of the other converters in that file, so I'm not sure how to rearrange that to reduce complexity :-/ Could you possibly put together a code sketch of what you mean?

[alamb]

Not really, because I am using the cast kernels in the Converter: 4b59fc9 (#8402) in the style of the other converters in that file

That code seems to be using cast as part of the implementation of casting dictionary keys, very similar to the way cast does internally: https://github.com/apache/arrow/blob/master/rust/arrow/src/compute/kernels/cast.rs#L979-L982

I was trying to suggest rather than making some sort of generic Convert<> function there was a function like:

convert_parquet_column(col_iter) --> ArrayRef

Where the choice of type for ArrayRef is entirely based on the type of col_iter (the parquet type, not the desired arrow type). And then use the cast kernel to go to the specific Arrow type that is needed

[nevi-me]

I've botched this branch a bit with my rebase on the parquet branch.
I rebased it against the parquet branch, but then I started getting stack overflows on datafusion and parquet.

(EDIT: I only see now that you mention the overflow on your last commit)

I've established that my safest path back to the parquet branch is to first rebase against one of my branches by nevi-me/arrow@ARROW-7842-cherry...integer32llc:dict

@carols10cents please let me know what you think.
I was trying to get everything up-to-date before I look at this PR locally (want to try Andrew's cast approach)

[nevi-me]

@carols10cents @alamb I think the whole reader logic needs replumbing ... There's at least a 1:1 mapping between Parquet types and Arrow types, and we can cast from Arrow types to other Arrow types based on the Arrow metadata. This is a less complex path, because one of the things I've been concerned about is that I/we are going to struggle a lot when we get to deeply-nested reads.

I previously didn't understand your needs re. dictionary support between Parquet > Arrow > DataFusion. I now have context, so I can make decisions better.

My plan was to remove trait CastRecordReader altogether, and instead use Arrow casts.
I prefer Arrow casts because they handle transparent casts of dyn Array & DataType::ANY instead of the combinatoral CastRecordReader.

I've now done this in integer32llc#3, but I left a lot of TODOs which I'd love for us to address so we don't carry the tech debt of cast converters.

The tests all pass now 🎊

[nevi-me]

My pleasure Carol, and thanks for cleaning up the cast converters

And yes, as you noted, I cherry-picked the "We need a custom comparison of ArrayData" commit from your ARROW-7842-cherry branch so that more tests would work on this branch. Do you think that commit is ready to go, even if the other commits on that branch aren't?

Yea, I really should get back to #8200. We need to make breaking changes to truly fix it, so I should disable the tests that still fail, so we can merge it in. I'll prioritise that this week.

I'm happy with this PR, subject to removing the C++ comment, and a clean CI (unless it's something minor).

I think after this, we should move work to the main branch for the rest of the Parquet <> Arrow IO 👍🏾

[carols10cents]

@nevi-me Rebased and fixed the last few things! I pulled the comment fix into its own PR, thanks for the tip on CI. Hoping for all greens now!

[nevi-me]

Merged