davidm-db commented on code in PR #46206: URL: https://github.com/apache/spark/pull/46206#discussion_r1579032798
########## sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala: ########## @@ -1150,12 +1166,11 @@ case class StringTrim(srcStr: Expression, trimStr: Option[Expression] = None) override protected def direction: String = "BOTH" - override def doEval(srcString: UTF8String): UTF8String = srcString.trim() + override def doEval(srcString: UTF8String): UTF8String = + CollationSupport.StringTrim.exec(srcString, collationId) override def doEval(srcString: UTF8String, trimString: UTF8String): UTF8String = - srcString.trim(trimString) - - override val trimMethod: String = "trim" Review Comment: It was only used in `doGenCode` in the superclass. After we changed to use `CollationSupport` classes/methods, and not call methods from `UTF8String` directly, there is no need for this field anymore. Earlier this field was used in `doGenCode` to call specific method of `UTF8String`. Now, if we kept same behavior to call methods from `CollationSupport` we would either need to have a lot of hardcoded string compares or use reflection to invoke methods. I think that this behavior where we match classes in `doGenCode` and then call respective methods from `CollationSupport` is more performant and robust. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
