[SPARK-48700][SQL] Mode expression for complex types (all collations) #47154

GideonPotok · 2024-06-30T21:37:48Z

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.

GideonPotok · 2024-07-10T02:41:28Z

@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.

GideonPotok · 2024-07-10T23:06:33Z

@uros-db

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala

sql/core/src/test/scala/org/apache/spark/sql/CollationSQLExpressionsSuite.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala

sql/core/src/test/scala/org/apache/spark/sql/CollationSQLExpressionsSuite.scala

GideonPotok · 2024-09-20T13:50:35Z

@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.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala

sql/core/src/test/scala/org/apache/spark/sql/CollationSQLExpressionsSuite.scala

…ssionsSuite.scala Co-authored-by: Maxim Gekk <[email protected]>

sql/core/src/test/scala/org/apache/spark/sql/CollationSQLExpressionsSuite.scala

MaxGekk · 2024-09-21T07:56:32Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala

+        CollationFactory.getCollationKey(data.asInstanceOf[UTF8String], st.collationId)
+      case _ =>
+        throw new SparkUnsupportedOperationException(
+          "UNSUPPORTED_MODE_DATA_TYPE",


UNSUPPORTED_MODE_DATA_TYPE is a sub-condition of DATATYPE_MISMATCH. You cannot refers to it in this way. Could you add a test for this case, please.

BTW, if you cannot reproduce the error via public API, we should consider to convert it to an internal error.

@MaxGekk For the test, I suppose the situation where a type is not binary stable and is not covered by checkInputDataTypes would include some UDTs? checkInputType just confirms it is not a MapType (a blacklist/blocklist), whereas collationAwareTransform is an allowlist/whitelist).

@MaxGekk @uros-db I am having a lot of trouble with this one! I implemented it as COMPLEX_EXPRESSION_UNSUPPORTED_INPUT.NO_INPUT, and plan to create a different subclass that represents this situation (Maybe BAD_INPUT), once I just have it working. But When I throw the following:

SparkUnsupportedOperationException( errorClass = "COMPLEX_EXPRESSION_UNSUPPORTED_INPUT.NO_INPUT",

I end up getting:

// org.apache.spark.SparkException: [INTERNAL_ERROR] Cannot // find sub error class 'COMPLEX_EXPRESSION_UNSUPPORTED_INPUT.NO_INPUT' SQLSTATE: XX000

Yet if I do

SparkUnsupportedOperationException( errorClass = "COMPLEX_EXPRESSION_UNSUPPORTED_INPUT",

Then org.apache.spark.ErrorClassesJsonReader#getMessageTemplate fails during the assertion assert(errorInfo.subClass.isDefined == subErrorClass.isDefined)

As the subClass is missing.

Would either of you be able to tell me if this is the right pattern (eg. maybe it should be ComplexExpressionException("BAD_INPUT"), not sure what is the "old" pattern and which is the preferred one).

@MaxGekk please re-review

…ly formatted' in SparkThrowableSuite

GideonPotok · 2024-10-01T11:46:56Z

@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)

MaxGekk · 2024-10-01T12:12:07Z

@GideonPotok Thank you for the ping. I will review this PR soon.

MaxGekk · 2024-10-01T13:09:08Z

+1, LGTM. Merging to master.
Thank you, @GideonPotok and @uros-db for review.

### 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. Closes apache#47154 from GideonPotok/collationmodecomplex. Lead-authored-by: Gideon P <[email protected]> Co-authored-by: Gideon Potok <[email protected]> Signed-off-by: Max Gekk <[email protected]>

GideonPotok changed the title ~~complex~~ [WIP] Jun 30, 2024

github-actions bot added the SQL label Jun 30, 2024

GideonPotok changed the title ~~[WIP]~~ [WIP] [SPARK-46837] [SPARK-48700] Mode expression for complex types (all collations) Jul 2, 2024

GideonPotok force-pushed the collationmodecomplex branch from 129768e to 92f284a Compare July 9, 2024 21:21

GideonPotok changed the title ~~[WIP] [SPARK-46837] [SPARK-48700] Mode expression for complex types (all collations)~~ [SPARK-46837] [SPARK-48700] Mode expression for complex types (all collations) Jul 9, 2024

GideonPotok force-pushed the collationmodecomplex branch from 92f284a to f469c2a Compare July 9, 2024 21:22

GideonPotok marked this pull request as ready for review July 10, 2024 02:35

GideonPotok changed the title ~~[SPARK-46837] [SPARK-48700] Mode expression for complex types (all collations)~~ [SPARK-48700] [SQL] Mode expression for complex types (all collations) Jul 10, 2024

uros-db reviewed Jul 14, 2024

View reviewed changes