[GitHub] [arrow] paveltiunov commented on pull request #9040: ARROW-11055: [Rust] [DataFusion] Support date_trunc function

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

paveltiunov commented on pull request #9040:
URL: https://github.com/apache/arrow/pull/9040#issuecomment-753430934


   Hey @jhorstmann ! Thanks for the review!
   
   > * In postgres the argument order is the other way around [`date_trunc('week', timestamp)`](https://www.postgresql.org/docs/13/functions-datetime.html#FUNCTIONS-DATETIME-TRUNC). I haven't compared with other databases, but following postgres makes sense to me most of the time :)
   
   I've swapped arguments order. It's 50/50 among databases however agree Postgres order is a little bit more common.
   
   > * A few unit tests, especially for the week truncation around the beginning/end of a year would be nice. Maybe the test input could be prepared using `to_timestamp` so the test is easier to review.
   
   I've added some tests at the beginning of the year. Is it something you were looking for? 
   
   > * In most usages, the date part parameter would be a literal and it might be worthwhile to optimize for that. Doesn't fit that nicely into the current `BuiltinScalarFunction` infrastructure, so I'm fine with considering that as out of scope for now.
   
   Yep. Agree it's out of scope for this PR. 
   
   @alamb I rebased it against the latest master.


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