[GitHub] [arrow] thisisnic edited a comment on pull request #10598: ARROW-13054: [C++] Add TemporalComponentExtractionOptions

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

thisisnic edited a comment on pull request #10598:
URL: https://github.com/apache/arrow/pull/10598#issuecomment-869885932


   > > @rok you mention this PR now only has `index_of_monday`, but in the diff I currently see `start_index`, which is something else?
   > > Also the explanation of it, "Index of the first day of the week", is a bit ambiguous (what is the first day of the week?)
   > 
   > @jorisvandenbossche well the beginning of the week could be e.g.: `Monday=0`, `Monday=1`, `Sunday=0` or `Sunday=1`. Meaning we really need two parameters:
   > 
   >     * which day (Sunday/Monday) starts the week - this is normally (eventually) given by locale but in the scope of this PR it's Monday.
   > 
   >     * what index does the week start at - this would here be set by `start_index`
   > 
   > 
   > Alternatively we can have `index_of_monday` and calculate kinda like `(day_of_week(timestamp) + index_of_monday) % 7`.
   > 
   > How about `"Index of the first day of the week"` to `"Offset of day of week"`?
   
   I'm not sure if this might be a little confusing combining them.  Could we perhaps define ourselves which integer maps to which day, and then just have a single parameter, `start_index`, similarly to how `week_start` works in R's lubridate package (https://lubridate.tidyverse.org/reference/day.html)?


-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org