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:
[email protected]
