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: add linear search for the interval function #19543

Merged
merged 9 commits into from
Sep 4, 2020

Conversation

ichn-hu
Copy link
Contributor

@ichn-hu ichn-hu commented Aug 27, 2020

What problem does this PR solve?

Issue Number: close #18525

Problem Summary: due to the binary search algorithm for the interval function we currently employed can't handle NULL values correctly, it caused different result from MySQL. After research the MySQL's implementation for this function, I figured out that MySQL uses linear search if there is a NULL argument, therefore I added this functionality to our interval function.

What is changed and how it works?

What's Changed: linear search is added for interval functions.

How it Works: we use linear search instead of binary search if any of the argument might be NULL.

Related changes

  • Need to cherry-pick to the 4.0 and 3.0.

Check List

Tests

  • Integration test

Side effects

  • Performance regression
    • Consumes more CPU

Release note

  • No release note.

@ichn-hu ichn-hu requested a review from a team as a code owner August 27, 2020 11:58
@ichn-hu ichn-hu requested review from lzmhhh123 and removed request for a team August 27, 2020 11:58
@ichn-hu ichn-hu force-pushed the add-linear-search-for-interval branch from 4dca148 to 5d8dc02 Compare August 27, 2020 12:02
@ichn-hu
Copy link
Contributor Author

ichn-hu commented Aug 28, 2020

/run-all-tests

2 similar comments
@ichn-hu
Copy link
Contributor Author

ichn-hu commented Aug 29, 2020

/run-all-tests

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Aug 31, 2020

/run-all-tests

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Aug 31, 2020

@XuHuaiyu Hi Huaiyu~ PTAL~

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Aug 31, 2020

@SunRunAway PTAL as well~

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Sep 1, 2020

@XuHuaiyu @SunRunAway friendly ping~

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Sep 1, 2020

/uncc @lzmhhh123

@ti-srebot ti-srebot removed the request for review from lzmhhh123 September 1, 2020 08:57
@ichn-hu
Copy link
Contributor Author

ichn-hu commented Sep 1, 2020

/cc @SunRunAway

@ti-srebot ti-srebot requested a review from SunRunAway September 1, 2020 08:58
@ichn-hu
Copy link
Contributor Author

ichn-hu commented Sep 1, 2020

/cc @XuHuaiyu

@ti-srebot ti-srebot requested a review from XuHuaiyu September 1, 2020 08:58
Copy link
Contributor

@SunRunAway SunRunAway 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-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 1, 2020
@SunRunAway
Copy link
Contributor

You can try to fix the coprocessor version.

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Sep 2, 2020

@XuHuaiyu PTAL

Copy link
Contributor

@XuHuaiyu XuHuaiyu 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-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Sep 3, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Sep 3, 2020
@ichn-hu
Copy link
Contributor Author

ichn-hu commented Sep 3, 2020

/merge

@ti-srebot
Copy link
Contributor

@ichn-hu Oops! auto merge is restricted to Committers of the SIG.See the corresponding SIG page for more information. Related SIGs: execution(slack).

@SunRunAway
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 3, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@ichn-hu merge failed.

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Sep 3, 2020

/run-unit-test

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Sep 3, 2020

/run-integration-test

@ichn-hu
Copy link
Contributor Author

ichn-hu commented Sep 3, 2020

/run-all-tests

@XuHuaiyu
Copy link
Contributor

XuHuaiyu commented Sep 4, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 5b26588 into pingcap:master Sep 4, 2020
ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Sep 4, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-3.0 in PR #19789

ti-srebot pushed a commit to ti-srebot/tidb that referenced this pull request Sep 4, 2020
@ti-srebot
Copy link
Contributor

cherry pick to release-4.0 in PR #19790

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression 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
Development

Successfully merging this pull request may close these issues.

INTERVAL function returns incorrect results & wrong warning
4 participants