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

Support other weekdays as start-of-the-week #50887

Open
sam97 opened this issue Jun 12, 2023 · 4 comments
Open

Support other weekdays as start-of-the-week #50887

sam97 opened this issue Jun 12, 2023 · 4 comments
Labels

Comments

@sam97
Copy link

sam97 commented Jun 12, 2023

Currently, toWeek() and its sister functions take mode as an argument which sets either Sunday or Monday as start of the week for date calculations. Clickhouse can take this to its conclusion by allowing users to set any day of the week as start of the week (SotW).

It is not uncommon to find organisations and even countries that have days like Saturday as SotW. Some professional teams work in shifts and define their own week structures, which have more than just Sunday and Monday as SotW. Given that it is not much effort, there are fairly good reasons to support any weekday as SotW. Almost all modern date libraries come with this feature.

mode is how SotW is defined, and it can easily be expanded upon. Per the source code, mode is an unsigned 8-bit integer. To represent seven days we only need three bits, one of which is already being used for Sunday/Monday. I think we can use two more of the remaining bits to extend the date functions without breaking backwards compatibility.

I'd already briefly started working on this feature, but I thought I'll post here before I continue.

Personally, I feel the mode way is quite clunky, and adding five more days, paired with the other two week-related options for each of them (range and week 1), will only make it clunkier. But I've settled with adapting this approach in the name of backwards compatibility. If there are plans to make breaking changes in this area in the near future, then please let me know so I can draft an alternate approach with a cleaner interface.

ANSI SQL Compatibility

The main reason I'm posting this right away is because I was sent here from the Slack channel for inquiring about ANSI SQL compatibility. I was told that date functions do fall under the SQL standard and that I should consult the core team before I make strides with this implementation.

As I don't have access to the standard, I'd like someone from Clickhouse to let me know if this approach is allowed under the SQL standard we follow. That'd be much appreciated before I spend any more effort.

@sam97 sam97 added the feature label Jun 12, 2023
@UnamedRus
Copy link
Contributor

UnamedRus commented Jun 12, 2023

Related #30420

@sam97
Copy link
Author

sam97 commented Jun 18, 2023

Thanks, I noticed that there is some inconsistency in the default SotW config, and that some functions don't allow setting it at all, like toStartOfInterval and date_diff (#45583).

I'm wondering if the proper solution is to break backwards compatibility and move all week-related configurations like SotW, range and week 1 to system/user settings. That is commonly how it is handled by a lot of packages to avoid repetition. But I don't know what the implications of breaking backwards compatibility are.

Maybe both can be done at once? Most mode arguments seem to be optional, so if they're omitted by the user their settings could be checked first before assuming the current defaults (0 for toWeek, 1 for toStartOfInterval, etc.).

Of course, all this depends on what the SQL standards for week-related functions are and how much Clickhouse wants to adhere to them.

@sam97
Copy link
Author

sam97 commented Feb 8, 2024

Getting my hands on the SQL standard (without paying a large sum) has proven fruitless. Taking a quick glance at how other database systems handle week numbers and SotW does not reveal any particular similarity among them. The only similarity I could find is the mode argument, and not in every DBMS.

I understand that the SQL standard cannot be shared on an open forum, so based on my little bit of research I'm willing to proceed with the approach below.

I think that, since quite a few other functions like toStartOfInterval could benefit with SotW, introducing a setting which indicates which week mode to use across all functions that require SotW is the right way forward. I think it is possible to keep backwards compatibility as well as the different defaults used by varying functions while allowing users to override them with the settings, or not if they choose not to. Moreover, spending effort to expand the mode argument as suggested by me initially does not seem important anymore if there is a setting already.

I will first compile a list of all functions and areas that this new setting should ideally impact, and jot down their defaults, whether they take mode as argument, whether mode is optiona or not, etc. so that the scope of the implementation is clearer.

As usual, any hints that can avoid me spending useless effort like breaking SQL standards compatibility is much appreciated.

@UnamedRus
Copy link
Contributor

BTW, another possible solution to problem: #56738

But, it's more general

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants