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

types: support %X %V %W formats for STR_TO_DATE() #21887

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

MySQL:
image

TiDB:
image

Problem Summary:

%X %V %W formats are not supported

What is changed and how it works?

What's Changed:

Support %X %V %W for STR_TO_DATE()

How it Works:

It works the same with MySQL after this change.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test

Release note

  • Support %X %V and %W format for builtin function STR_TO_DATE()

@tiancaiamao tiancaiamao marked this pull request as ready for review December 29, 2020 02:43
@tiancaiamao tiancaiamao requested a review from a team as a code owner December 29, 2020 02:43
@tiancaiamao tiancaiamao requested review from wshwsh12 and removed request for a team December 29, 2020 02:43
@tiancaiamao
Copy link
Contributor Author

PTAL @wshwsh12 @qw4990

@qw4990 qw4990 self-requested a review December 29, 2020 03:00
@zz-jason
Copy link
Member

@tiancaiamao it's a critical bug that produces incorrect result, could you file an issue first?

@tiancaiamao
Copy link
Contributor Author

tiancaiamao commented Mar 29, 2021

It's a tiny tiny bug fix for an ONCALL issue long long time ago.
I guess %X %V %W is rarely used by our user, so nobody care about this, and nobody even review the PR.
I don't think it's a critical one, otherwise it should not be treated like this.

Comment on lines +3239 to +3303
func yearOfWeekX(t *CoreTime, input string, ctx map[string]int) (string, bool) {
v, succ := parseDigits(input, 4)
if !succ {
return input, false
}
t.setYear(uint16(v))
ctx["X"] = 1
return input[4:], true
}

func yearOfWeekx(t *CoreTime, input string, ctx map[string]int) (string, bool) {
v, succ := parseDigits(input, 4)
if !succ {
return input, false
}
t.setYear(uint16(v))
ctx["x"] = 1
return input[4:], true
}

func weekModeV(t *CoreTime, input string, ctx map[string]int) (string, bool) {
v, succ := parseDigits(input, 2)
// v should be in [01, 53]
if !succ || v <= 0 || v > 53 {
return input, false
}
ctx["WeekNumber"] = v
ctx["SundayFirst"] = 1
ctx["V"] = 1
return input[2:], true
}

func weekModeU(t *CoreTime, input string, ctx map[string]int) (string, bool) {
v, succ := parseDigits(input, 2)
// v should be in [00, 53]
if !succ || v < 0 || v > 53 {
return input, false
}
ctx["WeekNumber"] = v
ctx["SundayFirst"] = 1
ctx["U"] = 1
return input[2:], true
}

func weekModev(t *CoreTime, input string, ctx map[string]int) (string, bool) {
v, succ := parseDigits(input, 2)
// v should be in [01, 53]
if !succ || v <= 0 || v > 53 {
return input, false
}
ctx["WeekNumber"] = v
ctx["v"] = 1
return input[2:], true
}

func weekModeu(t *CoreTime, input string, ctx map[string]int) (string, bool) {
v, succ := parseDigits(input, 2)
// v should be in [00, 53]
if !succ || v < 0 || v > 53 {
return input, false
}
ctx["WeekNumber"] = v
ctx["u"] = 1
return input[2:], true
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that most of the codes are duplicates, can we simplify them?

Copy link
Contributor Author

@tiancaiamao tiancaiamao Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, not duplicated.
Take a close look, you will find the tiny different between those functions. @wjhuang2016

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Can we take the difference as a parameter? Will it hurt the performance?

@ti-chi-bot ti-chi-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 10, 2021
@qw4990 qw4990 removed their request for review June 21, 2021 03:27
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2021
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2021
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2021
@tisonkun tisonkun added sig/sql-infra SIG: SQL Infra and removed sig/community-infra labels Sep 1, 2021
@ti-chi-bot
Copy link
Member

[FORMAT CHECKER NOTIFICATION]

Notice: To remove the do-not-merge/needs-linked-issue label, please provide the linked issue number on one line in the PR body, for example: Issue Number: close #123 or Issue Number: ref #456.

📖 For more info, you can check the "Contribute Code" section in the development guide.

@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 12, 2022
@wshwsh12 wshwsh12 removed their request for review November 5, 2022 05:40
@ti-chi-bot ti-chi-bot bot deleted a comment from ti-chi-bot Dec 19, 2023
Copy link

ti-chi-bot bot commented Nov 18, 2024

@tiancaiamao: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-integration-ddl-test dd46e59 link true /test pull-integration-ddl-test
idc-jenkins-ci-tidb/build dd46e59 link true /test build
idc-jenkins-ci-tidb/unit-test dd46e59 link true /test unit-test
idc-jenkins-ci-tidb/mysql-test dd46e59 link true /test mysql-test
idc-jenkins-ci-tidb/check_dev_2 dd46e59 link true /test check-dev2
idc-jenkins-ci-tidb/check_dev dd46e59 link true /test check-dev
pull-mysql-client-test dd46e59 link true /test pull-mysql-client-test
pull-br-integration-test dd46e59 link true /test pull-br-integration-test
pull-lightning-integration-test dd46e59 link true /test pull-lightning-integration-test
pull-integration-e2e-test dd46e59 link true /test pull-integration-e2e-test

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression do-not-merge/needs-linked-issue sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants