apache
[SPARK-48700] [SQL] Mode expression for complex types (all collations)
What changes were proposed in this pull request?
Add support for complex types with subfields that are collated strings, for the mode operator.
Why are the changes needed?
Full support for collations as per SPARK-48700
Does this PR introduce any user-facing change?
Yes.
How was this patch tested?
Unit tests only, so far.
Was this patch authored or co-authored using generative AI tooling?
No.
@uros-db This is ready for initial review. Let me know what you think. I am working on simplifying the implementation -- suggestions for how to do so are welcome. All suggestions welcome.
left some comments, code structure looks much better than before in my opinion
let's just clean this up a bit - not leave behind any comments from prototyping tests, and be a bit more careful with the naming overall
also, will we add support for MapType?
Re MapType: I think we shouldn't. I don't have as much capacity right now, so i prefer to keep it simple. Does that sound okay? I will let you know once all the suggested cleanup is in.
@uros-db , I just wanted to loop you in that I am just going to put finishing this PR on hold for like two weeks while I attend to some other responsibilities. Work is pretty crazy right now. Is that okay?
@uros-db I think this is ready for re-review lmk what you think.
The one thing I have to do is add a comment about following up with maptype. Where should that go?
Other changes and suggestions have been incorporated
@GideonPotok could you please address @MaxGekk's comments
@uros-db absolutely! Will let you know soon that all points are addressed.
@MaxGekk so sorry about that! New computer, thus default IntelliJ settings for auto formatting. But it is all fixed now.
Do you have some default IDE settings in a file form I could import into mine, for next time? If not I will adjust the settings manually before creating my next spark PR.
@MaxGekk please review (new error condition and test is up and running as was requested -- I can change the name and details of the new error condition, now that it and the test are all set up, just let me know if it needs a rename)
+1, LGTM. Merging to master. Thank you, @GideonPotok and @uros-db for review.