Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

week iso related function #20511

Closed
wangsimo0 opened this issue Mar 29, 2023 · 5 comments · Fixed by #26240
Closed

week iso related function #20511

wangsimo0 opened this issue Mar 29, 2023 · 5 comments · Fixed by #26240
Assignees

Comments

@wangsimo0
Copy link
Contributor

As mentioned in #15385, the week function in sr is not following ISO week.

There are two week related function needs week_iso function:

  1. weekiso: this is an iso version of weekofyear
  2. dayofweekiso: this is an iso version of dayofweek
@qmengss
Copy link
Contributor

qmengss commented Apr 13, 2023

Hi,could you assign this task to me please?

@qmengss
Copy link
Contributor

qmengss commented Apr 21, 2023

My idea is to implement the weekiso function, which requires taking Monday as the start of each week. Originally, I thought about implementing it simply by calling week_of_year_with_mode directly. However, this results in the following output for the weekiso function:

mysql> select week(event_time),week_iso(event_time),event_time from detail where event_time>'2023-01-01 00:00:00';
+------------------+---------------------+---------------------+
| week(event_time) | week_iso(event_time)| event_time          |
+------------------+---------------------+---------------------+
|                1 |                   1 | 2023-01-05 00:05:00 |
|                2 |                   2 | 2023-01-09 00:09:00 |
|                1 |                   1 | 2023-01-02 00:02:00 |
|                1 |                   1 | 2023-01-06 00:06:00 |
|                1 |                   1 | 2023-01-03 00:03:00 |
|                1 |                   1 | 2023-01-07 00:07:00 |
|                2 |                   2 | 2023-01-10 00:10:00 |
|                1 |                   0 | 2023-01-01 00:01:00 |
|                1 |                   1 | 2023-01-04 00:04:00 |
|                2 |                   1 | 2023-01-08 00:08:00 |
+------------------+---------------------+---------------------+

However, this does not seem to be compliant with ISO. Therefore, when the week_of_year_with_mode function is called with a date and a parameter of 1, we need to recalculate using the last day of the previous year as the parameter when the return value is 0. We should get 52 weeks for the previous year, and the output should be as follows:

+------------------+---------------------+---------------------+
| week(event_time) | week_iso(event_time)| event_time          |
+------------------+---------------------+---------------------+
|                1 |                   1 | 2023-01-05 00:05:00 |
|                2 |                   2 | 2023-01-09 00:09:00 |
|                1 |                   1 | 2023-01-02 00:02:00 |
|                1 |                   1 | 2023-01-06 00:06:00 |
|                1 |                   1 | 2023-01-03 00:03:00 |
|                1 |                   1 | 2023-01-07 00:07:00 |
|                2 |                   2 | 2023-01-10 00:10:00 |
|                1 |                  52 | 2023-01-01 00:01:00 |
|                1 |                   1 | 2023-01-04 00:04:00 |
|                2 |                   1 | 2023-01-08 00:08:00 |
+------------------+---------------------+---------------------+

Do you think this logic is feasible? @wangsimo0

@LiShuMing
Copy link
Contributor

Therefore, when the week_of_year_with_mode function is called with a date and a parameter of 1, we need to recalculate using the last day of the previous year as the parameter when the return value is 0. We should get 52 weeks for the previous year

My idea is to implement the weekiso function, which requires taking Monday as the start of each week. Originally, I thought about implementing it simply by calling week_of_year_with_mode directly. However, this results in the following output for the weekiso function:

mysql> select week(event_time),week_iso(event_time),event_time from detail where event_time>'2023-01-01 00:00:00';
+------------------+---------------------+---------------------+
| week(event_time) | week_iso(event_time)| event_time          |
+------------------+---------------------+---------------------+
|                1 |                   1 | 2023-01-05 00:05:00 |
|                2 |                   2 | 2023-01-09 00:09:00 |
|                1 |                   1 | 2023-01-02 00:02:00 |
|                1 |                   1 | 2023-01-06 00:06:00 |
|                1 |                   1 | 2023-01-03 00:03:00 |
|                1 |                   1 | 2023-01-07 00:07:00 |
|                2 |                   2 | 2023-01-10 00:10:00 |
|                1 |                   0 | 2023-01-01 00:01:00 |
|                1 |                   1 | 2023-01-04 00:04:00 |
|                2 |                   1 | 2023-01-08 00:08:00 |
+------------------+---------------------+---------------------+

However, this does not seem to be compliant with ISO. Therefore, when the week_of_year_with_mode function is called with a date and a parameter of 1, we need to recalculate using the last day of the previous year as the parameter when the return value is 0. We should get 52 weeks for the previous year, and the output should be as follows:

+------------------+---------------------+---------------------+
| week(event_time) | week_iso(event_time)| event_time          |
+------------------+---------------------+---------------------+
|                1 |                   1 | 2023-01-05 00:05:00 |
|                2 |                   2 | 2023-01-09 00:09:00 |
|                1 |                   1 | 2023-01-02 00:02:00 |
|                1 |                   1 | 2023-01-06 00:06:00 |
|                1 |                   1 | 2023-01-03 00:03:00 |
|                1 |                   1 | 2023-01-07 00:07:00 |
|                2 |                   2 | 2023-01-10 00:10:00 |
|                1 |                  52 | 2023-01-01 00:01:00 |
|                1 |                   1 | 2023-01-04 00:04:00 |
|                2 |                   1 | 2023-01-08 00:08:00 |
+------------------+---------------------+---------------------+

Do you think this logic is feasible? @wangsimo0

Sounds great. Can you pull a request for your anlysis?

qmengss added a commit to qmengss/starrocks that referenced this issue Apr 29, 2023
qmengss added a commit to qmengss/starrocks that referenced this issue Apr 29, 2023
Signed-off-by: qmengss <707549024@qq.com>
qmengss added a commit to qmengss/starrocks that referenced this issue May 1, 2023
Signed-off-by: qmengss <707549024@qq.com>
qmengss added a commit to qmengss/starrocks that referenced this issue May 1, 2023
Signed-off-by: qmengss <707549024@qq.com>

Signed-off-by: qmengss <707549024@qq.com>
@qmengss
Copy link
Contributor

qmengss commented May 1, 2023

Therefore, when the week_of_year_with_mode function is called with a date and a parameter of 1, we need to recalculate using the last day of the previous year as the parameter when the return value is 0. We should get 52 weeks for the previous year

My idea is to implement the weekiso function, which requires taking Monday as the start of each week. Originally, I thought about implementing it simply by calling week_of_year_with_mode directly. However, this results in the following output for the weekiso function:

mysql> select week(event_time),week_iso(event_time),event_time from detail where event_time>'2023-01-01 00:00:00';
+------------------+---------------------+---------------------+
| week(event_time) | week_iso(event_time)| event_time          |
+------------------+---------------------+---------------------+
|                1 |                   1 | 2023-01-05 00:05:00 |
|                2 |                   2 | 2023-01-09 00:09:00 |
|                1 |                   1 | 2023-01-02 00:02:00 |
|                1 |                   1 | 2023-01-06 00:06:00 |
|                1 |                   1 | 2023-01-03 00:03:00 |
|                1 |                   1 | 2023-01-07 00:07:00 |
|                2 |                   2 | 2023-01-10 00:10:00 |
|                1 |                   0 | 2023-01-01 00:01:00 |
|                1 |                   1 | 2023-01-04 00:04:00 |
|                2 |                   1 | 2023-01-08 00:08:00 |
+------------------+---------------------+---------------------+

However, this does not seem to be compliant with ISO. Therefore, when the week_of_year_with_mode function is called with a date and a parameter of 1, we need to recalculate using the last day of the previous year as the parameter when the return value is 0. We should get 52 weeks for the previous year, and the output should be as follows:

+------------------+---------------------+---------------------+
| week(event_time) | week_iso(event_time)| event_time          |
+------------------+---------------------+---------------------+
|                1 |                   1 | 2023-01-05 00:05:00 |
|                2 |                   2 | 2023-01-09 00:09:00 |
|                1 |                   1 | 2023-01-02 00:02:00 |
|                1 |                   1 | 2023-01-06 00:06:00 |
|                1 |                   1 | 2023-01-03 00:03:00 |
|                1 |                   1 | 2023-01-07 00:07:00 |
|                2 |                   2 | 2023-01-10 00:10:00 |
|                1 |                  52 | 2023-01-01 00:01:00 |
|                1 |                   1 | 2023-01-04 00:04:00 |
|                2 |                   1 | 2023-01-08 00:08:00 |
+------------------+---------------------+---------------------+

Do you think this logic is feasible? @wangsimo0

Sounds great. Can you pull a request for your anlysis?

hi, this is my PR, #22773.

qmengss added a commit to qmengss/starrocks that referenced this issue May 1, 2023
${PREPEND:SIGNED-OFF-BY=qmengss<707549024@qq.com>}
qmengss added a commit to qmengss/starrocks that referenced this issue May 1, 2023
${PREPEND:SIGNED-OFF-BY=qmengss<707549024@qq.com>}

Signed-off-by: qmengss <707549024@qq.com>
qmengss added a commit to qmengss/starrocks that referenced this issue May 1, 2023
qmengss added a commit to qmengss/starrocks that referenced this issue May 2, 2023
…e implementation logic. (StarRocks#20511)

Signed-off-by: qmengss <707549024@qq.com>
@qmengss
Copy link
Contributor

qmengss commented May 2, 2023

Therefore, when the week_of_year_with_mode function is called with a date and a parameter of 1, we need to recalculate using the last day of the previous year as the parameter when the return value is 0. We should get 52 weeks for the previous year

My idea is to implement the weekiso function, which requires taking Monday as the start of each week. Originally, I thought about implementing it simply by calling week_of_year_with_mode directly. However, this results in the following output for the weekiso function:

mysql> select week(event_time),week_iso(event_time),event_time from detail where event_time>'2023-01-01 00:00:00';
+------------------+---------------------+---------------------+
| week(event_time) | week_iso(event_time)| event_time          |
+------------------+---------------------+---------------------+
|                1 |                   1 | 2023-01-05 00:05:00 |
|                2 |                   2 | 2023-01-09 00:09:00 |
|                1 |                   1 | 2023-01-02 00:02:00 |
|                1 |                   1 | 2023-01-06 00:06:00 |
|                1 |                   1 | 2023-01-03 00:03:00 |
|                1 |                   1 | 2023-01-07 00:07:00 |
|                2 |                   2 | 2023-01-10 00:10:00 |
|                1 |                   0 | 2023-01-01 00:01:00 |
|                1 |                   1 | 2023-01-04 00:04:00 |
|                2 |                   1 | 2023-01-08 00:08:00 |
+------------------+---------------------+---------------------+

However, this does not seem to be compliant with ISO. Therefore, when the week_of_year_with_mode function is called with a date and a parameter of 1, we need to recalculate using the last day of the previous year as the parameter when the return value is 0. We should get 52 weeks for the previous year, and the output should be as follows:

+------------------+---------------------+---------------------+
| week(event_time) | week_iso(event_time)| event_time          |
+------------------+---------------------+---------------------+
|                1 |                   1 | 2023-01-05 00:05:00 |
|                2 |                   2 | 2023-01-09 00:09:00 |
|                1 |                   1 | 2023-01-02 00:02:00 |
|                1 |                   1 | 2023-01-06 00:06:00 |
|                1 |                   1 | 2023-01-03 00:03:00 |
|                1 |                   1 | 2023-01-07 00:07:00 |
|                2 |                   2 | 2023-01-10 00:10:00 |
|                1 |                  52 | 2023-01-01 00:01:00 |
|                1 |                   1 | 2023-01-04 00:04:00 |
|                2 |                   1 | 2023-01-08 00:08:00 |
+------------------+---------------------+---------------------+

Do you think this logic is feasible? @wangsimo0

Sounds great. Can you pull a request for your anlysis?

Hi,I found that when calling week(datetime, mode) with mode set to 3, it conforms to the ISO week standard. So I simplified the logic by directly calling the underlying method of week with mode set to 3. This reduces the amount of code and achieves the same effect.

qmengss added a commit to qmengss/starrocks that referenced this issue May 4, 2023
…string case (StarRocks#20511)

Signed-off-by: qmengss <707549024@qq.com>
imay pushed a commit that referenced this issue May 4, 2023
Signed-off-by: qmengss <707549024@qq.com>
xiangguangyxg pushed a commit to xiangguangyxg/starrocks that referenced this issue May 10, 2023
@wangsimo0 wangsimo0 added the good first issue Good for newcomers label May 12, 2023
LiShuMing pushed a commit to LiShuMing/starrocks that referenced this issue May 26, 2023
LiShuMing pushed a commit to LiShuMing/starrocks that referenced this issue May 26, 2023
wanpengfei-git pushed a commit that referenced this issue May 26, 2023
Signed-off-by: Kevin Li <ming.moriarty@gmail.com>
numbernumberone pushed a commit to numbernumberone/starrocks that referenced this issue May 31, 2023
abc982627271 pushed a commit to abc982627271/starrocks that referenced this issue Jun 5, 2023
mergify bot pushed a commit that referenced this issue Jun 19, 2023
Signed-off-by: qmengss <707549024@qq.com>
(cherry picked from commit 054b2d6)

# Conflicts:
#	test/sql/test_time_fn/R/test_time_fn
#	test/sql/test_time_fn/T/test_time_fn
Youngwb pushed a commit to Youngwb/starrocks that referenced this issue Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants