ARROW-13561: [C++] Implement week kernel that accepts WeekOptions

[rok]

This is to resolve ARROW-13561.

[github-actions]

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

[rok]

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

[lidavidm]

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.

[rok]

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 MySQLDayOfWeekOptions.week_start should be settable either 1 or 7.

[lidavidm]

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

[rok]

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

[rok]

This evaluates to true in MySQL on sqlfiddle:

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[lidavidm]

Given ISOWeek is a special case of Week now, maybe we should just have one kernel (and perhaps some conveniences, e.g. DayOfWeekOptions::IsoWeek())?

[rok]

Oh yeah, that's a nice idea.
I'm still trying to exactly match WEEK from MySQL and once I do I'll look into this. Sorry for the WIP.

[lidavidm]

Oh, no worries. I can hold off looking at this until you're ready.

[rok]

Early feedback is great though! :)

[lidavidm]

Ping on this? Since it seems the two kernels are very similar.

[rok]

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

[rok]

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

Perhaps DayOfWeekOptions.count_full_weeks is more idiomatic.

[lidavidm]

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

[rok]

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[rok]

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?

[rok]

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

[rok]

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.

[lidavidm]

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(

[rok]

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?

[lidavidm]

Some nits on style/wording.

If you're up to add R tests, I think that would be appreciated, but otherwise we can put that in a follow-up JIRA (since there might be other work to bind the new kernels to dplyr features or whatnot).

[lidavidm]

Similarly here, can we put comments for the parameter literals? Or actually, it might be nice to define WeekOptions::IsoDefaults and WeekOptions::UsDefaults which could then be used here and in the R bindings.

[rok]

Added WeekOptions::ISODefaults and WeekOptions::USDefaults.

[lidavidm]

Also add a helper for USWeek?

[rok]

Done.

[lidavidm]

It appears to be missing its counterpart in api_scalar.cc.

[rok]

Added.

[lidavidm]

Thanks, LGTM.

[rok]

Thanks for the review @lidavidm ! :)

[pitrou]

Rebased, will merge

[rok]

Thanks @pitrou@ianmcook@jorisvandenbossche@lidavidm !

[rok]

This is to resolve ARROW-13561.

[github-actions]

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

[rok]

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

[lidavidm]

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.

[rok]

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 MySQLDayOfWeekOptions.week_start should be settable either 1 or 7.

[lidavidm]

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

[rok]

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

[rok]

This evaluates to true in MySQL on sqlfiddle:

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[lidavidm]

Given ISOWeek is a special case of Week now, maybe we should just have one kernel (and perhaps some conveniences, e.g. DayOfWeekOptions::IsoWeek())?

[rok]

Oh yeah, that's a nice idea.
I'm still trying to exactly match WEEK from MySQL and once I do I'll look into this. Sorry for the WIP.

[lidavidm]

Oh, no worries. I can hold off looking at this until you're ready.

[rok]

Early feedback is great though! :)

[lidavidm]

Ping on this? Since it seems the two kernels are very similar.

[rok]

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

[rok]

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

Perhaps DayOfWeekOptions.count_full_weeks is more idiomatic.

[lidavidm]

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

[rok]

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[rok]

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?

[rok]

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

[rok]

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.

[lidavidm]

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(

[rok]

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?

[lidavidm]

Some nits on style/wording.

If you're up to add R tests, I think that would be appreciated, but otherwise we can put that in a follow-up JIRA (since there might be other work to bind the new kernels to dplyr features or whatnot).

[lidavidm]

Similarly here, can we put comments for the parameter literals? Or actually, it might be nice to define WeekOptions::IsoDefaults and WeekOptions::UsDefaults which could then be used here and in the R bindings.

[rok]

Added WeekOptions::ISODefaults and WeekOptions::USDefaults.

[lidavidm]

Also add a helper for USWeek?

[rok]

Done.

[lidavidm]

It appears to be missing its counterpart in api_scalar.cc.

[rok]

Added.

[lidavidm]

Thanks, LGTM.

[rok]

Thanks for the review @lidavidm ! :)

[pitrou]

Rebased, will merge

[rok]

Thanks @pitrou@ianmcook@jorisvandenbossche@lidavidm !

[rok]

This is to resolve ARROW-13561.

[github-actions]

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

[rok]

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

[lidavidm]

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.

[rok]

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 MySQLDayOfWeekOptions.week_start should be settable either 1 or 7.

[lidavidm]

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

[rok]

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

[rok]

This evaluates to true in MySQL on sqlfiddle:

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[lidavidm]

Given ISOWeek is a special case of Week now, maybe we should just have one kernel (and perhaps some conveniences, e.g. DayOfWeekOptions::IsoWeek())?

[rok]

Oh yeah, that's a nice idea.
I'm still trying to exactly match WEEK from MySQL and once I do I'll look into this. Sorry for the WIP.

[lidavidm]

Oh, no worries. I can hold off looking at this until you're ready.

[rok]

Early feedback is great though! :)

[lidavidm]

Ping on this? Since it seems the two kernels are very similar.

[rok]

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

[rok]

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

Perhaps DayOfWeekOptions.count_full_weeks is more idiomatic.

[lidavidm]

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

[rok]

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[rok]

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?

[rok]

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

[rok]

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.

[lidavidm]

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(

[rok]

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?

[lidavidm]

Some nits on style/wording.

If you're up to add R tests, I think that would be appreciated, but otherwise we can put that in a follow-up JIRA (since there might be other work to bind the new kernels to dplyr features or whatnot).

[lidavidm]

Similarly here, can we put comments for the parameter literals? Or actually, it might be nice to define WeekOptions::IsoDefaults and WeekOptions::UsDefaults which could then be used here and in the R bindings.

[rok]

Added WeekOptions::ISODefaults and WeekOptions::USDefaults.

[lidavidm]

Also add a helper for USWeek?

[rok]

Done.

[lidavidm]

It appears to be missing its counterpart in api_scalar.cc.

[rok]

Added.

[lidavidm]

Thanks, LGTM.

[rok]

Thanks for the review @lidavidm ! :)

[pitrou]

Rebased, will merge

[rok]

Thanks @pitrou@ianmcook@jorisvandenbossche@lidavidm !

[rok]

This is to resolve ARROW-13561.

[github-actions]

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

[rok]

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

[lidavidm]

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.

[rok]

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 MySQLDayOfWeekOptions.week_start should be settable either 1 or 7.

[lidavidm]

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

[rok]

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

[rok]

This evaluates to true in MySQL on sqlfiddle:

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[lidavidm]

Given ISOWeek is a special case of Week now, maybe we should just have one kernel (and perhaps some conveniences, e.g. DayOfWeekOptions::IsoWeek())?

[rok]

Oh yeah, that's a nice idea.
I'm still trying to exactly match WEEK from MySQL and once I do I'll look into this. Sorry for the WIP.

[lidavidm]

Oh, no worries. I can hold off looking at this until you're ready.

[rok]

Early feedback is great though! :)

[lidavidm]

Ping on this? Since it seems the two kernels are very similar.

[rok]

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

[rok]

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

Perhaps DayOfWeekOptions.count_full_weeks is more idiomatic.

[lidavidm]

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

[rok]

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[rok]

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?

[rok]

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

[rok]

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.

[lidavidm]

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(

[rok]

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?

[lidavidm]

Some nits on style/wording.

If you're up to add R tests, I think that would be appreciated, but otherwise we can put that in a follow-up JIRA (since there might be other work to bind the new kernels to dplyr features or whatnot).

[lidavidm]

Similarly here, can we put comments for the parameter literals? Or actually, it might be nice to define WeekOptions::IsoDefaults and WeekOptions::UsDefaults which could then be used here and in the R bindings.

[rok]

Added WeekOptions::ISODefaults and WeekOptions::USDefaults.

[lidavidm]

Also add a helper for USWeek?

[rok]

Done.

[lidavidm]

It appears to be missing its counterpart in api_scalar.cc.

[rok]

Added.

[lidavidm]

Thanks, LGTM.

[rok]

Thanks for the review @lidavidm ! :)

[pitrou]

Rebased, will merge

[rok]

Thanks @pitrou@ianmcook@jorisvandenbossche@lidavidm !

[rok]

This is to resolve ARROW-13561.

[github-actions]

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

[rok]

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

[lidavidm]

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.

[rok]

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 MySQLDayOfWeekOptions.week_start should be settable either 1 or 7.

[lidavidm]

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

[rok]

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

[rok]

This evaluates to true in MySQL on sqlfiddle:

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[lidavidm]

Given ISOWeek is a special case of Week now, maybe we should just have one kernel (and perhaps some conveniences, e.g. DayOfWeekOptions::IsoWeek())?

[rok]

Oh yeah, that's a nice idea.
I'm still trying to exactly match WEEK from MySQL and once I do I'll look into this. Sorry for the WIP.

[lidavidm]

Oh, no worries. I can hold off looking at this until you're ready.

[rok]

Early feedback is great though! :)

[lidavidm]

Ping on this? Since it seems the two kernels are very similar.

[rok]

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

[rok]

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

Perhaps DayOfWeekOptions.count_full_weeks is more idiomatic.

[lidavidm]

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

[rok]

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[rok]

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?

[rok]

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

[rok]

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.

[lidavidm]

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(

[rok]

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?

[lidavidm]

Some nits on style/wording.

If you're up to add R tests, I think that would be appreciated, but otherwise we can put that in a follow-up JIRA (since there might be other work to bind the new kernels to dplyr features or whatnot).

[lidavidm]

Similarly here, can we put comments for the parameter literals? Or actually, it might be nice to define WeekOptions::IsoDefaults and WeekOptions::UsDefaults which could then be used here and in the R bindings.

[rok]

Added WeekOptions::ISODefaults and WeekOptions::USDefaults.

[lidavidm]

Also add a helper for USWeek?

[rok]

Done.

[lidavidm]

It appears to be missing its counterpart in api_scalar.cc.

[rok]

Added.

[lidavidm]

Thanks, LGTM.

[rok]

Thanks for the review @lidavidm ! :)

[pitrou]

Rebased, will merge

[rok]

Thanks @pitrou@ianmcook@jorisvandenbossche@lidavidm !

[rok]

This is to resolve ARROW-13561.

[github-actions]

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

[rok]

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

[lidavidm]

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.

[rok]

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 MySQLDayOfWeekOptions.week_start should be settable either 1 or 7.

[lidavidm]

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

[rok]

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

[rok]

This evaluates to true in MySQL on sqlfiddle:

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[lidavidm]

Given ISOWeek is a special case of Week now, maybe we should just have one kernel (and perhaps some conveniences, e.g. DayOfWeekOptions::IsoWeek())?

[rok]

Oh yeah, that's a nice idea.
I'm still trying to exactly match WEEK from MySQL and once I do I'll look into this. Sorry for the WIP.

[lidavidm]

Oh, no worries. I can hold off looking at this until you're ready.

[rok]

Early feedback is great though! :)

[lidavidm]

Ping on this? Since it seems the two kernels are very similar.

[rok]

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

[rok]

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

Perhaps DayOfWeekOptions.count_full_weeks is more idiomatic.

[lidavidm]

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

[rok]

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[rok]

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?

[rok]

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

[rok]

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.

[lidavidm]

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(

[rok]

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?

[lidavidm]

Some nits on style/wording.

If you're up to add R tests, I think that would be appreciated, but otherwise we can put that in a follow-up JIRA (since there might be other work to bind the new kernels to dplyr features or whatnot).

[lidavidm]

Similarly here, can we put comments for the parameter literals? Or actually, it might be nice to define WeekOptions::IsoDefaults and WeekOptions::UsDefaults which could then be used here and in the R bindings.

[rok]

Added WeekOptions::ISODefaults and WeekOptions::USDefaults.

[lidavidm]

Also add a helper for USWeek?

[rok]

Done.

[lidavidm]

It appears to be missing its counterpart in api_scalar.cc.

[rok]

Added.

[lidavidm]

Thanks, LGTM.

[rok]

Thanks for the review @lidavidm ! :)

[pitrou]

Rebased, will merge

[rok]

Thanks @pitrou@ianmcook@jorisvandenbossche@lidavidm !

[rok]

This is to resolve ARROW-13561.

[github-actions]

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

[rok]

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

[lidavidm]

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.

[rok]

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 MySQLDayOfWeekOptions.week_start should be settable either 1 or 7.

[lidavidm]

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

[rok]

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

[rok]

This evaluates to true in MySQL on sqlfiddle:

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[lidavidm]

Given ISOWeek is a special case of Week now, maybe we should just have one kernel (and perhaps some conveniences, e.g. DayOfWeekOptions::IsoWeek())?

[rok]

Oh yeah, that's a nice idea.
I'm still trying to exactly match WEEK from MySQL and once I do I'll look into this. Sorry for the WIP.

[lidavidm]

Oh, no worries. I can hold off looking at this until you're ready.

[rok]

Early feedback is great though! :)

[lidavidm]

Ping on this? Since it seems the two kernels are very similar.

[rok]

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

[rok]

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

Perhaps DayOfWeekOptions.count_full_weeks is more idiomatic.

[lidavidm]

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

[rok]

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[rok]

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?

[rok]

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

[rok]

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.

[lidavidm]

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(

[rok]

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?

[lidavidm]

Some nits on style/wording.

If you're up to add R tests, I think that would be appreciated, but otherwise we can put that in a follow-up JIRA (since there might be other work to bind the new kernels to dplyr features or whatnot).

[lidavidm]

Similarly here, can we put comments for the parameter literals? Or actually, it might be nice to define WeekOptions::IsoDefaults and WeekOptions::UsDefaults which could then be used here and in the R bindings.

[rok]

Added WeekOptions::ISODefaults and WeekOptions::USDefaults.

[lidavidm]

Also add a helper for USWeek?

[rok]

Done.

[lidavidm]

It appears to be missing its counterpart in api_scalar.cc.

[rok]

Added.

[lidavidm]

Thanks, LGTM.

[rok]

Thanks for the review @lidavidm ! :)

[pitrou]

Rebased, will merge

[rok]

Thanks @pitrou@ianmcook@jorisvandenbossche@lidavidm !

[rok]

This is to resolve ARROW-13561.

[github-actions]

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

[rok]

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

[lidavidm]

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.

[rok]

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 MySQLDayOfWeekOptions.week_start should be settable either 1 or 7.

[lidavidm]

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

[rok]

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

[rok]

This evaluates to true in MySQL on sqlfiddle:

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[lidavidm]

Given ISOWeek is a special case of Week now, maybe we should just have one kernel (and perhaps some conveniences, e.g. DayOfWeekOptions::IsoWeek())?

[rok]

Oh yeah, that's a nice idea.
I'm still trying to exactly match WEEK from MySQL and once I do I'll look into this. Sorry for the WIP.

[lidavidm]

Oh, no worries. I can hold off looking at this until you're ready.

[rok]

Early feedback is great though! :)

[lidavidm]

Ping on this? Since it seems the two kernels are very similar.

[rok]

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

[rok]

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

Perhaps DayOfWeekOptions.count_full_weeks is more idiomatic.

[lidavidm]

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

[rok]

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

Mode	First day of week	Range	Week 1 is the first week
0	Sunday	0-53	with a Sunday in this year
1	Monday	0-53	with 4 or more days this year
2	Sunday	1-53	with a Sunday in this year
3	Monday	1-53	with 4 or more days this year
4	Sunday	0-53	with 4 or more days this year
5	Monday	0-53	with a Monday in this year
6	Sunday	1-53	with 4 or more days this year
7	Monday	1-53	with a Monday in this year

[rok]

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?

[rok]

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

[rok]

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.

[lidavidm]

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(

[rok]

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?

[lidavidm]

Some nits on style/wording.

If you're up to add R tests, I think that would be appreciated, but otherwise we can put that in a follow-up JIRA (since there might be other work to bind the new kernels to dplyr features or whatnot).

[lidavidm]

Similarly here, can we put comments for the parameter literals? Or actually, it might be nice to define WeekOptions::IsoDefaults and WeekOptions::UsDefaults which could then be used here and in the R bindings.

[rok]

Added WeekOptions::ISODefaults and WeekOptions::USDefaults.

[lidavidm]

Also add a helper for USWeek?

[rok]

Done.

[lidavidm]

It appears to be missing its counterpart in api_scalar.cc.

[rok]

Added.

[lidavidm]

Thanks, LGTM.

[rok]

Thanks for the review @lidavidm ! :)

[pitrou]

Rebased, will merge

[rok]

Thanks @pitrou@ianmcook@jorisvandenbossche@lidavidm !