physical-plan: Cast nested group values back to dictionary if necessary

[brancz]

Which issue does this PR close?

Closes #12542

Rationale for this change

Non-nested arrays were already being dictionary encoded again if the input was dictionary encoded, so this just replicates that behavior for structs and lists.

What changes are included in this PR?

1. Check whether there are any nested dictionaries to encode.
2. Encode arrays that have nested dictionaries appropriately.

Note this may not be conclusive, I've only included lists and structs since those are the cases/types that I need and that I'm most familiar with. I think it'd be nice to get these changes in though and handle any further cases in separate PRs.

Are these changes tested?

Yes, added a unit test that tests precisely the scenario that led me to opening the issue.

Are there any user-facing changes?

No, only a bug fix. If the queries didn't panic before it could have been a change in behavior, but since all queries of this sort panicked before it's purely a bug fix.

@alamb @tustvold @andygrove

[alamb]

Thanks for the contribution @brancz -- I have a suggestion for how to simplify the code. Let me know if it makes sense

[alamb]

It seems to me like dictionary_encode_nested would work on non nested types as well, and thus you could simply always call it (and avoid the special case for Dict above too).

That which would make this code simpler and easier to follow. You can probably avoid the clone if you wanted (not a huge deal give it is an Arc):

Suggested change

	if expected.is_nested() && needs_nested_dictionary_encoding(expected, array)?
	{
	*array = dictionary_encode_nested(array.clone(), expected)?;
	}
	*array = dictionary_encode_nested(&array, expected)?;

[brancz]

Fair enough! I was optimizing for not attempting to dictionary encode a bit prematurely. Changed it!

[brancz]

Fixed all lints. Can you re-run tests?

[alamb]

BTW the code looks 👨‍🍳 to me -- once the CI passes I think this will be good to go

Also, once we have merged this CI runs on your future PR will run automaticallly

[brancz]

Also, once we have merged this CI runs on your future PR will run automatically

Awesome! 🥳

We have more things on our list that we'll work off over the next weeks/months, so that'll come in handy!

[brancz]

Huh weird, that last lint didn't fail for me locally, fixed it.

[brancz]

I'm really sorry, I think now it should truly be the last time that you need to re-run. I fixed it exactly the way clippy suggested.

[alamb]

Another thing that would make it slightly easier to review these PRs is if you pushed new commits rather than force pushing

All PRs get squash into a single commit on merge, so the commit history isn't preserved but makes it easy to see what has changed since the last revie

[brancz]

Got it!

[alamb]

🚀 thank you very much @brancz 🙏

here is hoping that the next few PRs will go more smoothly!