Expand date formatters and add `DATE_FORMAT` function for better MySQL compatibility by JakeBamrah · Pull Request #46302 · ClickHouse/ClickHouse

JakeBamrah · 2023-02-11T18:07:43Z

Changelog category:

Improvement

Changelog entry:

Add an alias "DATE_FORMAT()" for function "formatDateTime()" to improve compatibility with MySQL's SQL dialect, extend functionformatDateTime() with substitutions "a", "b", "c", "h", "i", "k", "l" "r", "s", "W".

Documentation entry for user-facing changes

User-readable short description:
DATE_FORMAT is an alias of formatDateTime. Formats a Time according to the given Format string. Format is a constant expression, so you cannot have multiple formats for a single result column. (Provide link to formatDateTime)

Motivation: Both DATE_FORMAT and the expanded date-formatter substitutions adhere more closely to MySQL standard behavior. Aligning clickhouse MySQL queries more closely with the MySQL standard will help users avoid unexpected MySQL date formatting behaviours.
Example use: SELECT DATE_FORMAT(now(), '%Y-%m-%d %H:%i:%s')

CLAassistant · 2023-02-11T18:07:53Z

All committers have signed the CLA.

JakeBamrah · 2023-02-13T12:40:26Z

@alexey-milovidov Hey! Sorry to bother you but I'm struggling with a particular test (00921_datetime64_compatibility_long.sh) and was wondering if you could provide some guidance please?

We are comparing a long timestamp based on metadata so it will never truly align as the metadata will always be behind a now() function call.

Full test failure log below:

Running 1 stateless tests (MainProcess).

00921_datetime64_compatibility_long:                                    [ FAIL ] - result differs with reference:
--- /var/www/dev/cpp/ClickHouse/tests/queries/0_stateless/00921_datetime64_compatibility_long.reference 2023-02-13 08:58:03.926449411 +0000
+++ /var/www/dev/cpp/ClickHouse/tests/queries/0_stateless/00921_datetime64_compatibility_long.stdout    2023-02-13 09:07:06.412794589 +0000
@@ -353,7 +353,7 @@
 "DateTime64(9, 'Europe/Minsk')","2019-09-16 19:20:11.000000000"
 "DateTime64(9, 'Europe/Minsk')","2019-09-16 19:20:11.234000000"
 ------------------------------------------
-SELECT formatDateTime(N, \'%C %d %D %e %F %H %I %j %m %M %p %R %S %T %u %V %w %y %Y %%\', \'Asia/Istanbul\')
+SELECT formatDateTime(N, \'%C %d %D %e %F %H %I %j %m %i %p %R %S %T %u %V %w %y %Y %%\', \'Asia/Istanbul\')
 "String","20 16 09/16/19 16 2019-09-16 00 12 259 09 00 AM 00:00 00 00:00:00 1 38 1 19 2019 %"
 "String","20 16 09/16/19 16 2019-09-16 19 07 259 09 20 PM 19:20 11 19:20:11 1 38 1 19 2019 %"
 "String","20 16 09/16/19 16 2019-09-16 19 07 259 09 20 PM 19:20 11 19:20:11 1 38 1 19 2019 %"


Settings used in the test: --max_insert_threads=0 --group_by_two_level_threshold=542407 --group_by_two_level_threshold_bytes=1 --distributed_aggregation_memory_efficient=1 --fsync_metadata=1 --output_format_parallel_formatting=1 --input_format_parallel_parsing=0 --min_chunk_bytes_for_parallel_parsing=15361772 --max_read_buffer_size=807949 --prefer_localhost_replica=0 --max_block_size=12007 --max_threads=21 --optimize_or_like_chain=0 --optimize_read_in_order=1 --read_in_order_two_level_merge_threshold=11 --optimize_aggregation_in_order=1 --aggregation_in_order_max_block_bytes=21126662 --min_compress_block_size=199654 --max_compress_block_size=1098516 --use_uncompressed_cache=1 --min_bytes_to_use_direct_io=10737418240 --min_bytes_to_use_mmap_io=1 --local_filesystem_read_method=pread --remote_filesystem_read_method=read --local_filesystem_read_prefetch=0 --remote_filesystem_read_prefetch=0 --compile_expressions=0 --compile_aggregate_expressions=1 --compile_sort_description=0 --merge_tree_coarse_index_granularity=2 --optimize_distinct_in_order=0 --optimize_sorting_by_input_stream_properties=0 --enable_memory_bound_merging_of_aggregation_results=1

MergeTree settings used in test: --prefer_fetch_merged_part_size_threshold=1 --vertical_merge_algorithm_min_rows_to_activate=1 --vertical_merge_algorithm_min_columns_to_activate=1 --min_merge_bytes_to_use_direct_io=10737418240 --index_granularity_bytes=10055451 --merge_max_block_size=13077 --index_granularity=45704 --min_bytes_for_wide_part=1051792190

Database: test_zni9p86b

Having 1 errors! 0 tests passed. 0 tests skipped. 5.32 s elapsed (MainProcess).
Won't run stateful tests because test data wasn't loaded.
All tests have finished.

rschu1ze

Thanks for this PR, I left a few comments but it looks overall good!

Kindly also update the docs (--> docs/en/sql-reference/functions/date-time-functions.md) with the new/changed substitutions and please add a hint about the new alias DATE_FORMAT.

src/Functions/formatDateTime.cpp

rschu1ze · 2023-02-14T09:37:10Z

src/Functions/formatDateTime.cpp

Semantics of "M" change (l. 983ff. on the left side) which technically makes this PR backwards-incompatible. This is okay but we should mark it as such - I'll add the right tags. It will then show up in the backward-incompatible section of the monthly changelog.

src/Functions/formatDateTime.cpp

tests/queries/0_stateless/00718_format_datetime.sql

rschu1ze · 2023-02-14T10:03:02Z

@JakeBamrah Consider to split this PR into two ... one PR with new substitutions (which I expect to pass the tests quickly) + another one with changed substitution "M".

According to test results, there are still some memory corruption errors (e.g. in the AST Fuzzer) - would you like to check these?

rschu1ze · 2023-02-14T10:10:28Z

About the test failure in 00921_datetime64_compatibility_long:

I see that you changed file tests/queries/0_stateless/00921_datetime64_compatibility_long.python ... it looks to me as if this is the cause for the failure.

JakeBamrah · 2023-02-14T20:07:06Z

@JakeBamrah Consider to split this PR into two ... one PR with new substitutions (which I expect to pass the tests quickly) + another one with changed substitution "M".

According to test results, there are still some memory corruption errors (e.g. in the AST Fuzzer) - would you like to check these?

Hey @rschu1ze! Thank you very much for taking the time to review. I think splitting this change into two separate PRs (as you've suggested) would be the best approach to take. I'll have a go and see how I get on 😊

JakeBamrah · 2023-02-15T20:57:39Z

@JakeBamrah Consider to split this PR into two ... one PR with new substitutions (which I expect to pass the tests quickly) + another one with changed substitution "M".

According to test results, there are still some memory corruption errors (e.g. in the AST Fuzzer) - would you like to check these?

Most of the initial changes made for the first PR (aside from replacing M date formatter). I realized the mistake I made which led to the AST Fuzzer test error. Hopefully it should be resolved with this latest commit a5ae8d3640 :)

rschu1ze · 2023-02-16T09:44:18Z

(Not a big thing but once feedback / a review exists in a PR, it is more convenient to push fixes as separate commits so it is visible what was changed. That's not possible with force-pushing.)

rschu1ze · 2023-02-16T09:45:36Z

Adjusted the changelog entry ... semantics of "M" will be changed in a separate PR.

JakeBamrah · 2023-02-16T11:38:40Z

(Not a big thing but once feedback / a review exists in a PR, it is more convenient to push fixes as separate commits so it is visible what was changed. That's not possible with force-pushing.)

Thanks for the heads up :) My intention was to keep the commit history tidy as a single commit but that's no big deal. I can push them as separate commits in future and squash before merge.

rschu1ze · 2023-02-16T11:40:57Z

Don't worry about the commit history, it is a mess already :) Complex PRs can also be integrated into the main branch by squashing them ... all intermediate steps will vanish then.

JakeBamrah · 2023-02-16T16:43:49Z

Don't worry about the commit history, it is a mess already :) Complex PRs can also be integrated into the main branch by squashing them ... all intermediate steps will vanish then.

Finally done it, all tests pass @rschu1ze. 🥳 Permission to get this change merged?

rschu1ze · 2023-02-16T16:55:09Z

Congrats to your first contribution to ClickHouse. May there be many more :)

JakeBamrah · 2023-02-16T17:09:56Z

Congrats to your first contribution to ClickHouse. May there be many more :)

My first contribution ever haha! I'm actually so happy! Thanks so much for your help throughout.

Would I be okay to start work on the M date format change and link it to the same issue?

rschu1ze · 2023-02-16T17:20:22Z

Definitely. As long as the changelog and docs entries are good, nobody is going to complain.

robot-ch-test-poll3 added the pr-improvement Pull request with some product improvements label Feb 11, 2023

JakeBamrah force-pushed the master branch from 503a17c to 2bad686 Compare February 11, 2023 18:08

JakeBamrah changed the title ~~#46184: Expand date formatters and add DATE_FORMAT function for MySQL~~ Add function DATE_FORMAT as a compatibility alias. #46184 Feb 11, 2023

JakeBamrah changed the title ~~Add function DATE_FORMAT as a compatibility alias. #46184~~ #46184: Expand date formatters and add DATE_FORMAT function for MySQL Feb 11, 2023

alexey-milovidov added the can be tested Allows running workflows for external contributors label Feb 12, 2023

JakeBamrah force-pushed the master branch from 2bad686 to edc0871 Compare February 13, 2023 09:09

rschu1ze self-assigned this Feb 13, 2023

rschu1ze changed the title ~~#46184: Expand date formatters and add DATE_FORMAT function for MySQL~~ Expand date formatters and add DATE_FORMAT function for MySQL Feb 14, 2023

rschu1ze changed the title ~~Expand date formatters and add DATE_FORMAT function for MySQL~~ Expand date formatters and add DATE_FORMAT function for better MySQL compatibility Feb 14, 2023

rschu1ze added the pr-backward-incompatible Pull request with backwards incompatible changes label Feb 14, 2023

rschu1ze changed the title ~~Expand date formatters and add DATE_FORMAT function for better MySQL compatibility~~ Expand date formatters and add DATE_FORMAT function for better MySQL compatibility Feb 14, 2023

rschu1ze reviewed Feb 14, 2023

View reviewed changes

JakeBamrah force-pushed the master branch from edc0871 to bfba098 Compare February 14, 2023 22:01

robot-ch-test-poll3 removed the pr-backward-incompatible Pull request with backwards incompatible changes label Feb 14, 2023

JakeBamrah force-pushed the master branch 3 times, most recently from a5ae8d3 to 9f7643e Compare February 15, 2023 20:55

JakeBamrah force-pushed the master branch from 9f7643e to 4c4e495 Compare February 16, 2023 09:40

add DATE_FORMAT alias and remaining mySQL date formatters

4ea814a

JakeBamrah force-pushed the master branch from 4c4e495 to 4ea814a Compare February 16, 2023 09:52

rschu1ze merged commit 0ff404d into ClickHouse:master Feb 16, 2023

lizhou1111 mentioned this pull request May 9, 2024

Feature/issue 688 support function parse_datetime timeplus-io/proton#693

Merged

10 tasks

Conversation

JakeBamrah commented Feb 11, 2023 • edited by rschu1ze Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category:

Changelog entry:

Documentation entry for user-facing changes

Uh oh!

CLAassistant commented Feb 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakeBamrah commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rschu1ze left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rschu1ze Feb 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rschu1ze commented Feb 14, 2023

Uh oh!

rschu1ze commented Feb 14, 2023

Uh oh!

JakeBamrah commented Feb 14, 2023

Uh oh!

JakeBamrah commented Feb 15, 2023

Uh oh!

rschu1ze commented Feb 16, 2023

Uh oh!

rschu1ze commented Feb 16, 2023

Uh oh!

JakeBamrah commented Feb 16, 2023

Uh oh!

rschu1ze commented Feb 16, 2023

Uh oh!

JakeBamrah commented Feb 16, 2023

Uh oh!

rschu1ze commented Feb 16, 2023

Uh oh!

JakeBamrah commented Feb 16, 2023

Uh oh!

rschu1ze commented Feb 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

JakeBamrah commented Feb 11, 2023 •

edited by rschu1ze

Loading

CLAassistant commented Feb 11, 2023 •

edited

Loading

JakeBamrah commented Feb 13, 2023 •

edited

Loading