https://issues.apache.org/jira/browse/ARROW-13561

Shall we also change DayOfWeekOptions to WeekOptions? It would be a better fit overall if this PR is merged.

So essentially, ISOWeek starts from 1, Week starts from 0, and neither use DayOfWeekOptions.week_start? In that case I would just make them two optionless kernels.

To get parity with MySQL DayOfWeekOptions.week_start should be settable either 1 or 7.

So essentially, ISOWeek starts from 1, Week starts from 0, and neither use DayOfWeekOptions.week_start? In that case I would just make them two optionless kernels.

To get parity with MySQL DayOfWeekOptions.week_start should be settable either 1 or 7.

Ah, ok. But unless I'm mistaken, week_start is not actually used by the week kernel.

Ah, ok. But unless I'm mistaken, week_start is not actually used by the week kernel.

I'm just trying to understand if it does :)

This evaluates to true in MySQL on sqlfiddle:

SELECT WEEK('2008-1-6', 1) != WEEK('2008-1-6', 4)

@ianmcook This now covers MySQL WEEK modes 1, 3, 4 and 6 (week 1 is the first week with 4 or more days this year).
If we want to cover modes 0, 2, 5, and 7 (week 1 is the first week with a Monday/Sunday in this year) we could add another parameter e.g.: DayOfWeekOptions.week_start_in_year. Do you think we should?

Another thing to note here is that DayOfWeekOptions.one_based_numbering doesn't exactly mean we count from 0 or 1 but rather that a date from iso week 52 of previous year that is in this year will be counted as 0 instead of 52. It's a MySQL convention.

we could to add another parameter e.g.: DayOfWeekOptions.week_start_in_year.

Perhaps DayOfWeekOptions.count_full_weeks is more idiomatic.

If the option doesn't apply to DayOfWeek, maybe it should be a separate WeekOptions in that case.

So then we now have:
DayOfWeekOptions with week_start and one_based_numbering

We would need to add three binary parameters to match the 8 modes of MySQL:

• WeekOptions with week_start (first day of week),count_from_zero (range), count_full_weeks (week 1 is ..)

I tried adding WeekOptions with the following parameters:

  /// What day does the week start with (Monday=true, Sunday=false)
  bool week_starts_monday;
  /// Days in current year that fall into last years ISO week return week 0 if true
  bool count_from_zero;
  /// Is the first week fully in the the year or only its 4 or more days
  bool first_week_in_year;

I've also added tests with all the MySQL modes. Docs and the logic need some more work, but first: do we go this way or stick to iso week only?

@lidavidm @pitrou I've removed the ISOWeek kernel and added iso_week and us_week functions that use week with specific options.

R CI is reporting:

── Error (test-dplyr-lubridate.R:80:3): extract isoweek from timestamp ─────────
Error: Invalid: Attempted to initialize KernelState from null FunctionOptions
Backtrace:
     █
  1. ├─arrow:::expect_dplyr_equal(...) test-dplyr-lubridate.R:80:2
  2. │ ├─testthat::expect_warning(...) helper-expectation.R:101:4
  3. │ │ └─testthat:::quasi_capture(enquo(object), label, capture_warnings)
  4. │ │   ├─testthat:::.capture(...)
  5. │ │   │ └─base::withCallingHandlers(...)
  6. │ │   └─rlang::eval_bare(quo_get_expr(.quo), quo_get_env(.quo))
  7. │ └─rlang::eval_tidy(expr, rlang::new_data_mask(rlang::env(input = record_batch(tbl))))
  8. ├─input %>% mutate(x = isoweek(datetime)) %>% collect()
  9. ├─dplyr::collect(.)
 10. └─arrow:::collect.arrow_dplyr_query(.)
 11.   └─arrow:::do_exec_plan(x)
 12.     └─plan$Build(.data)
 13.       └─node$Project(projection)
 14.         ├─self$preserve_sort(ExecNode_Project(self, cols, names(cols)))
 15.         └─arrow:::ExecNode_Project(self, cols, names(cols))
── Error (test-dplyr-lubridate.R:205:3): extract isoweek from date ─────────────
Error: Invalid: Attempted to initialize KernelState from null FunctionOptions

I suppose we then need ISOWeek and USWeek declared with options in the API.

Yeah, we'd need a case here:

Yeah, we'd need a case here:

arrow/r/src/compute.cpp

Line 129 in 075c6c6

std::shared_ptr<arrow::compute::FunctionOptions> make_compute_options(

@lidavidm oh, didn't realize it had to be there.
Maybe would be good to add R tests as well then?

Thanks for the review @lidavidm ! :)

Thanks @pitrou @ianmcook @jorisvandenbossche @lidavidm !