uros-db commented on code in PR #46597:
URL: https://github.com/apache/spark/pull/46597#discussion_r1635247796
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Mode.scala:
##########
@@ -74,16 +90,25 @@ case class Mode(
if (buffer.isEmpty) {
return null
}
-
+ val collationAwareBuffer = child.dataType match {
+ case c: StringType if
+ !CollationFactory.fetchCollation(c.collationId).supportsBinaryEquality
=>
+ val collationId = c.collationId
+ val modeMap = buffer.toSeq.groupMapReduce {
Review Comment:
I left some comments in https://github.com/apache/spark/pull/46917 -
overall, I'm liking the new approach, but I agree with you that group mapReduce
was far less invasive... Personally, I'm fine with either approach and don't
have a hunch on why one might obviously prevail over the other at this point,
but there might be good value in drilling deeper and running benchmarks on
https://github.com/apache/spark/pull/46917 - then we would be able to compare,
discuss, and opt for our winner
After all, we shouldn't spend much more time here, so I suggest the
following - if you're feeling up for it @GideonPotok please continue
investigating further with the new approach (OHM) until we reach benchmark
results for a final decision. If not, I would be happy with cleaning everything
up with the old approach (GMR). After all, we can always re-visit this in the
future
How does this sound? @GideonPotok @dbatomic
--
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: reviews-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
