[GitHub] [spark] viirya commented on issue #24557: [SPARK-27653][SQL] Add max_by() SQL aggregate function

[GitBox <gi...@apache.org>]

viirya commented on issue #24557: [SPARK-27653][SQL] Add max_by() SQL aggregate function
URL: https://github.com/apache/spark/pull/24557#issuecomment-490746455
 
 
   @JoshRosen Thanks for the review!
   
   > * Could you also implement min_by(x, y)?
   Yes. I originally planed to have separate PR for it. I'm fine to add it here.  A shared abstract superclass to share code is good.
   
   > * Presto also has three-argument versions of max_by / min_by:
   Agreed. We don't need three-argument versions now. If we need it, we can add it in a followup.
   
   > * Were there any bugs in older implementations of Presto version that we might have replicated here? Or Presto tests for edge-cases that we could emulate?
   
   * For using rows / structs as the ordering value, I also think it would work. I will add few tests.
   * For null ordering values, I already have few test cases. I checked Presto's results and they are matched. Let me see double-check if we've covered the same edge-case.
   
   
   

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org