add toStartOfWeek #1942

dust1 · 2021-09-23T12:12:44Z

I hereby agree to the terms of the CLA available at: https://databend.rs/policies/cla/

Summary

Summary about this PR

Changelog

Improvement

Related Issues

#853

Test Plan

Unit Tests ok

Stateless Tests ok

databend-bot · 2021-09-23T12:12:47Z

Thanks for the contribution!
I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

codecov-commenter · 2021-09-23T12:48:19Z

Codecov Report

Merging #1942 (c9deea5) into master (3df848d) will decrease coverage by 1%.
The diff coverage is 1%.

@@           Coverage Diff            @@
##           master   #1942     +/-   ##
========================================
- Coverage      70%     69%     -2%     
========================================
  Files         635     643      +8     
  Lines       38218   36125   -2093     
========================================
- Hits        26865   25010   -1855     
+ Misses      11353   11115    -238

Impacted Files	Coverage Δ
common/functions/src/scalars/dates/week_date.rs	`0% <0%> (ø)`
common/functions/src/scalars/dates/date.rs	`61% <100%> (+<1%)`	⬆️
common/functions/src/scalars/dates/date_test.rs	`91% <100%> (ø)`
store/src/configs/config.rs	`87% <0%> (-4%)`	⬇️
common/datavalues/src/macros.rs	`30% <0%> (-3%)`	⬇️
common/management/src/user/utils.rs	`68% <0%> (-3%)`	⬇️
common/datavalues/src/data_field.rs	`55% <0%> (-2%)`	⬇️
common/runtime/src/runtime_test.rs	`98% <0%> (-2%)`	⬇️
query/src/sql/plan_parser.rs	`73% <0%> (-1%)`	⬇️
common/management/src/user/user_mgr_test.rs	`97% <0%> (-1%)`	⬇️
... and 227 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3df848d...c9deea5. Read the comment docs.

BohuTANG · 2021-09-23T23:28:33Z

/review @sundy-li

databend-bot · 2021-09-23T23:28:35Z

Take the reviewer to sundy-li

sundy-li · 2021-09-24T01:38:03Z

common/functions/src/scalars/dates/week_date.rs

+        let week_mode = mode.unwrap_or(0);
+        let mut weekday = value.weekday().number_from_monday();
+        if (week_mode & 1) == 0 {
+            weekday = value.weekday().number_from_sunday();


If week_mode is 1,should use monday first?

bool monday_first_mode = week_mode & static_cast<UInt8>(WeekModeFlag::MONDAY_FIRST); if (monday_first_mode) { return toFirstDayNumOfWeek(v); }

I find clickhouse doc, if week_mode is 1,First day of week is Monday. https://clickhouse.com/docs/zh/sql-reference/functions/date-time-functions/#toweekdatemode

So this should be like:

let mut weekday = value.weekday().number_from_sunday(); if week_mode & 1 { weekday = value.weekday().number_from_monday(); }

?

shouldn't I have 1 in the code? This is magic number?

No, just a little weird.

Default weekday should be number_from_sunday because of the week_mode defaults to 0.

I understand.I will modify it right away

databend-bot · 2021-09-24T14:56:02Z