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.

Sign up for GitHub

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

Jump to bottom

expression: enable dayname/monthname function push down for tiflash #32595

Merged
merged 50 commits into from
Feb 25, 2022
Merged

expression: enable dayname/monthname function push down for tiflash #32595

merged 50 commits into from
Feb 25, 2022

Conversation

yibin87
Copy link
Contributor

@yibin87 yibin87 commented Feb 24, 2022 •
edited
Loading

What problem does this PR solve?

Issue Number: close #32594

Problem Summary:

What is changed and how it works?

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

Enable dayname/monthname function push down for tiflash.

yibin hu and others added 30 commits December 23, 2021 15:06
@yibin87
Fix greatest/least issue when handling duration type
a2dc3eb
@yibin87
Fix some greatest/least function incorrect behavior when mixed types
29185d6
@yibin87
Fix greatest/least issue when handling duration type
4a29734
@yibin87
merge changes from trunk
56e033c
@yibin87
Separate GreatestTime and GreatestDate, LeastTime and LeastDate, and …
545dec2 
…add integration tests
@yibin87
Merge branch 'master' of github.com:yibin87/tidb
5ffc3a6
@yibin87
Merge branch 'master' into master
a4d5db5
@yibin87
Add support for vector greatest/least functions
fb273c7
@yibin87
Merge branch 'master' of github.com:yibin87/tidb
2dac27c
@yibin87
make format
e1574b1
@yibin87
Format enum definitions
5c4aef3
@yibin87
Format enum definitions
6ad60a3
@yibin87
Format enum definitions
3003a03
@yibin87
Format change
9e571ff
@yibin87
Format change
18244f0
@yibin87
Format change
f1fc032
@yibin87
Refact code and add vec test cases
367ab6e
@yibin87
Merge branch 'master' into master
630983e
@yibin87
Fmt changes
b745867
@yibin87
Modify comment to trigger unit test
1e11ca0
@yibin87
Adjust import sequence
d4505c3
@ti-chi-bot
Merge branch 'master' into master
3ba6f5d
@ti-chi-bot
Merge branch 'master' into master
de2240f
@ti-chi-bot
Merge branch 'master' into master
a6addf6
@ti-chi-bot
Merge branch 'master' into master
fa47deb
@yibin87
Fix mysql test json case
88fd4d6
@yibin87
Merge branch 'master' of github.com:yibin87/tidb
6bbb43d
@yibin87
Merge branch 'master' into master
6ba4c28
@yibin87
Add JSON tests
356c372
@yibin87
Merge branch 'master' of github.com:yibin87/tidb
2fabae4
@yibin87
Copy link
Contributor Author

yibin87 commented Feb 24, 2022

/cc @windtalker @fzhedu

@yibin87
Copy link
Contributor Author

yibin87 commented Feb 24, 2022

/run-all-tests

windtalker
windtalker reviewed Feb 24, 2022
View reviewed changes
Copy link
Contributor

@windtalker windtalker left a comment

Choose a reason for hiding this comment

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

Should add some ut for this.

@ti-chi-bot ti-chi-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 24, 2022
@yibin87
Copy link
Contributor Author

yibin87 commented Feb 24, 2022

Should add some ut for this.

UT added

@yibin87 yibin87 requested a review from windtalker February 24, 2022 05:59
@sre-bot
Copy link
Contributor

sre-bot commented Feb 24, 2022 •
edited
Loading

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/b5232362484db221b7677da2fdc8845d2f66aa3a

ngaut
ngaut approved these changes Feb 24, 2022
View reviewed changes
Copy link
Member

@ngaut ngaut left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 24, 2022
@windtalker
Copy link
Contributor

I suggest to squash the commits.

@yibin87 yibin87 requested a review from ngaut February 24, 2022 09:46
windtalker
windtalker approved these changes Feb 24, 2022
View reviewed changes
Copy link
Contributor

@windtalker windtalker left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 24, 2022
fzhedu
fzhedu reviewed Feb 24, 2022
View reviewed changes
Copy link
Contributor

@fzhedu fzhedu left a comment

Choose a reason for hiding this comment

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

please check whether the function is related to sql_mode?

@yibin87
Copy link
Contributor Author

yibin87 commented Feb 25, 2022

please check whether the function is related to sql_mode?

Didn't find any sql mode related to these two functions. Their behaviors are controlled by lc_time_names variable. However, it is a read only variable in TiDB.

@windtalker
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 3fc5eee

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 25, 2022
@ti-chi-bot
Copy link
Member

@yibin87: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

@yibin87
Copy link
Contributor Author

yibin87 commented Feb 25, 2022

/run-mysql-test

@ti-chi-bot ti-chi-bot merged commit a680d66 into pingcap:master Feb 25, 2022
@ti-chi-bot ti-chi-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fzhedu fzhedu fzhedu left review comments

@windtalker windtalker windtalker approved these changes

@ngaut ngaut Awaiting requested review from ngaut

Assignees
No one assigned
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support DayName/MonthName function push down for TiFlash
7 participants
@yibin87 @ti-chi-bot @sre-bot @windtalker @ngaut @fzhedu @hawkingrei