fix: Optimize some functions to rewrite dictionary-encoded strings

[vaibhawvipul]

Which issue does this PR close?

Closes #504 .

Rationale for this change

Improves performance

What changes are included in this PR?

rewrite dictionary encoded strings

How are these changes tested?

All test cases pass.

[andygrove]

Thanks @vaibhawvipul. This is looking good. There is also this section in cast.rs:

    fn cast_array(&self, array: ArrayRef) -> DataFusionResult<ArrayRef> {
        let to_type = &self.data_type;
        let array = array_with_timezone(array, self.timezone.clone(), Some(to_type))?;
        let from_type = array.data_type().clone();

        // unpack dictionary string arrays first
        // TODO: we are unpacking a dictionary-encoded array and then performing
        // the cast. We could potentially improve performance here by casting the
        // dictionary values directly without unpacking the array first, although this
        // would add more complexity to the code

[vaibhawvipul]

Thanks @vaibhawvipul. This is looking good. There is also this section in cast.rs:

    fn cast_array(&self, array: ArrayRef) -> DataFusionResult<ArrayRef> {
        let to_type = &self.data_type;
        let array = array_with_timezone(array, self.timezone.clone(), Some(to_type))?;
        let from_type = array.data_type().clone();

        // unpack dictionary string arrays first
        // TODO: we are unpacking a dictionary-encoded array and then performing
        // the cast. We could potentially improve performance here by casting the
        // dictionary values directly without unpacking the array first, although this
        // would add more complexity to the code

This is done @andygrove . Thank you for pointing this out.

[andygrove]

There is a test failure:

Cause: org.apache.comet.CometNativeException: Unsupported data type Dictionary(Int32, Binary) for function md5

[vaibhawvipul]

There is a test failure:

Cause: org.apache.comet.CometNativeException: Unsupported data type Dictionary(Int32, Binary) for function md5

fixed

[andygrove]

I don't know if we would currently hit the use case of casting to a dictionary type, or whether this is something that could help us in the future (it seems like we could take advantage of this when running natively even though Spark doesn't support keeping data dictionary-encoded after a cast, as far as I know, but maybe @viirya could confirm).

[vaibhawvipul]

awaiting @viirya 's comment before making any change.

[andygrove]

LGTM. Thanks for refactoring the code @vaibhawvipul, it is looking much cleaner now. I would like to see the additional test that I mentioned before we merge this.

[andygrove]

I think this is ready to merge, but I'd like to get a another review. Perhaps @parthchandra @huaxingao @kazuyukitanimura or @viirya could take a look

[parthchandra]

I don't see a logic change here. Is this just refactoring?

[vaibhawvipul]

yes, no logic change here.

[parthchandra]

lgtm

[andygrove]

Thanks @vaibhawvipul and thanks for the review @parthchandra