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

expression: refine date_add/sub return type and precision #35009

Merged
merged 60 commits into from
Jun 16, 2022

Conversation

zanmato1984
Copy link
Contributor

@zanmato1984 zanmato1984 commented May 27, 2022

What problem does this PR solve?

Issue Number: close #31799, close #9813. And part of #31867 will be resolved by this PR.

Problem Summary:

There exists several issues about function date_add and date_sub:

  1. Return types of several signatures don't match MySQL's (doc):
    1. date_add/sub(int/double/decimal, interval): MySQL returns string, TiDB returns datetime.
    2. date_add/sub(duration, interval) when interval unit contains YMD part (except for DAY_MICROSECOND - I believe it's a mismatch between MySQL's actual behavior and its document, but it doesn't matter much), MySQL returns datetime with current date padded, TiDB returns time.
    3. Precision of the return type doesn't match MySQL's in many cases. Note that here I mean the precision concluded by type inference - MySQL client option --column-type-info is your good friend.
  2. Runtime doesn't respect the precision inferred in compile time. For a datetime value, we check if the MS part is zero then adjust the precision to 0 (min fsp) or 6 (max fsp). No respect to compile time type at all.

Honestly I can file 10+ bugs easily about the above issues.

What is changed and how it works?

Don't get scared of this huge PR - 90% are tests and 80% of which are generated, the core is quite simple. This PR includes the following changes:

  1. The core change is in expression/builtin_time.go:
    1. Refine the type inference about the return type and precision of date_add and date_sub.
    2. Refine the evaluation so that the type and precision by type inference is respected.
    3. Add necessary signatures - ideally we should be able to leverage implicit cast to get a datetime value from int/double/decimal/string, however, as described in this comment, to comply with MySQL's weird behaviors we need specialized overload for each of int/double/decimal/string.
    4. Abstract things like "add or sub", "how to evaluate the datetime value", and "how to evaluate the interval value". Use some simple composition to eliminate the mostly-copy-paste individual overloads. And the overall LOC is reduced from 1456 to 591.
  2. The vectorized versions are simple enough to not use generator, thus moved from generator (expression/generator/time_vec.go) to regular implementations (expression/builtin_time_vec.go). I would have to write more code in template than just directly write them in go. However the vectorized tests are enhanced by adding more cases.
  3. Tests:
    1. Add thorough type infer test cases.
    2. Update several existing UT and integration test cases that already mismatch MySQL's result - each of them is a bug-to-be-reported.
    3. Add more cases to existing UT and integration test.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Fix the issue that the return type and precision of function `date_add` and `date_sub` does not match that of MySQL.

@ti-chi-bot
Copy link
Member

ti-chi-bot commented May 27, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • mjonss
  • winoros

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 needs-cherry-pick-release-5.2 needs-cherry-pick-release-5.3 Type: Need cherry pick to release-5.3 needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-6.0 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 27, 2022
@zanmato1984 zanmato1984 added type/bugfix This PR fixes a bug. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 needs-cherry-pick-release-5.2 needs-cherry-pick-release-5.3 Type: Need cherry pick to release-5.3 needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-6.0 labels May 27, 2022
@ti-chi-bot ti-chi-bot added needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 needs-cherry-pick-release-5.2 needs-cherry-pick-release-5.3 Type: Need cherry pick to release-5.3 needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. needs-cherry-pick-6.0 size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 27, 2022
@zanmato1984
Copy link
Contributor Author

/merge

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 020b32a

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 16, 2022
@zanmato1984
Copy link
Contributor Author

/run-mysql-test tidb-test=pr/1734

@zanmato1984
Copy link
Contributor Author

/rebuild

1 similar comment
@zanmato1984
Copy link
Contributor Author

/rebuild

@hawkingrei
Copy link
Member

/run-build

@ti-chi-bot ti-chi-bot merged commit 4ae78cd into pingcap:master Jun 16, 2022
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 16, 2022
@ti-srebot
Copy link
Contributor

cherry pick to release-5.0 in PR #35444

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 16, 2022
@ti-srebot
Copy link
Contributor

cherry pick to release-5.1 in PR #35445

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 16, 2022
@ti-srebot
Copy link
Contributor

cherry pick to release-5.2 in PR #35446

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 16, 2022
@ti-srebot
Copy link
Contributor

cherry pick to release-5.3 in PR #35447

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 16, 2022
@ti-srebot
Copy link
Contributor

cherry pick to release-5.4 in PR #35448

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Jun 16, 2022
@ti-srebot
Copy link
Contributor

cherry pick to release-6.0 in PR #35449

@sre-bot
Copy link
Contributor

sre-bot commented Jun 16, 2022

TiDB MergeCI notify

🔴 Bad News! New failing [1] after this pr merged.
These new failed integration tests seem to be caused by the current PR, please try to fix these new failed integration tests, thanks!

CI Name Result Duration Compare with Parent commit
idc-jenkins-ci-tidb/common-test 🟥 failed 2, success 10, total 12 10 min New failing
idc-jenkins-ci-tidb/integration-common-test 🔴 failed 2, success 9, total 11 18 min Existing failure
idc-jenkins-ci/integration-cdc-test 🟢 all 34 tests passed 31 min Existing passed
idc-jenkins-ci-tidb/tics-test 🟢 all 1 tests passed 12 min Existing passed
idc-jenkins-ci-tidb/integration-ddl-test 🟢 all 6 tests passed 7 min 59 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2 🟢 all 28 tests passed 7 min 4 sec Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1 🟢 all 26 tests passed 6 min 32 sec Existing passed
idc-jenkins-ci-tidb/mybatis-test 🟢 all 1 tests passed 3 min 38 sec Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test 🟢 all 1 tests passed 3 min 25 sec Existing passed
idc-jenkins-ci-tidb/plugin-test 🟢 build success, plugin test success 4min Existing passed

@zanmato1984 zanmato1984 deleted the issue-31799 branch June 16, 2022 09:02
@ti-chi-bot ti-chi-bot removed the needs-cherry-pick-release-5.4 Should cherry pick this PR to release-5.4 branch. label Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-cherry-pick-release-5.0 needs-cherry-pick-release-5.1 needs-cherry-pick-release-5.2 needs-cherry-pick-release-5.3 Type: Need cherry pick to release-5.3 release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
7 participants