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

optimize Date format #1337

Merged
merged 12 commits into from
Jan 7, 2021
Merged

optimize Date format #1337

merged 12 commits into from
Jan 7, 2021

Conversation

windtalker
Copy link
Contributor

@windtalker windtalker commented Jan 5, 2021 •
edited by JaySon-Huang
Loading

What problem does this PR solve?

Problem Summary:

DateFormat is slow in TiFlash, need to optimize it.

What is changed and how it works?

What's Changed:

How it Works:

  1. avoid all sprintf in MyTimeBase::dateFormat
  2. precompile format string to avoid parsing format string everytime
  3. pre-allocate && reuse string capacity in FunctionDateFormat

Result:

on TPCH-20, with batchcop

query with optimize without optimize
select count(date_format(L_SHIPDATE, '%Y-%m-%d')) from lineitem 1.26 sec 5.14 sec

Related changes

Check List

Tests

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

Side effects

Release note

  • optimize the performance of date_format function in TiFlash

@windtalker windtalker requested a review from hanfei1991 January 5, 2021 09:00
leiysky
leiysky reviewed Jan 5, 2021
View reviewed changes
dbms/src/Common/MyTime.cpp Show resolved Hide resolved
dbms/src/Common/MyTime.cpp Show resolved Hide resolved
dbms/src/Common/MyTime.cpp Show resolved Hide resolved
hanfei1991
hanfei1991 reviewed Jan 6, 2021
View reviewed changes
Copy link
Member

@hanfei1991 hanfei1991 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 Jan 6, 2021
@windtalker
Copy link
Contributor Author

/run-all-tests

@leiysky
Copy link
Contributor

leiysky commented Jan 6, 2021

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Jan 6, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Jan 6, 2021
@windtalker windtalker added the status/can-merge Indicates a PR has been approved by a committer. label Jan 6, 2021
@ti-srebot
Copy link
Collaborator

/run-all-tests

@ti-srebot
Copy link
Collaborator

@windtalker merge failed.

@windtalker windtalker added needs-cherry-pick-release-4.0 PR which needs to be cherry-picked to release-4.0 needs-cherry-pick-release-5.0-rc PR which needs to be cherry-picked to release-5.0-rc labels Jan 7, 2021
@windtalker
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Collaborator

/run-all-tests

@ti-srebot
Copy link
Collaborator

@windtalker merge failed.

@windtalker
Copy link
Contributor Author

/run-all-tests

@windtalker windtalker merged commit f452ab3 into pingcap:master Jan 7, 2021
@windtalker windtalker deleted the date_format_opt branch January 7, 2021 05:32
This was referenced Jan 7, 2021
Merged
Merged
ti-srebot added a commit that referenced this pull request Jan 20, 2021
@ti-srebot
optimize Date format (#1337) (#1339)
c0e1b09
ti-srebot added a commit that referenced this pull request Jan 20, 2021
@ti-srebot
optimize Date format (#1337) (#1340)
c9c9a1a
@JaySon-Huang JaySon-Huang mentioned this pull request May 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leiysky leiysky leiysky left review comments

@hanfei1991 hanfei1991 hanfei1991 left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees
No one assigned
Labels
needs-cherry-pick-release-4.0 PR which needs to be cherry-picked to release-4.0 needs-cherry-pick-release-5.0-rc PR which needs to be cherry-picked to release-5.0-rc 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.
4 participants
@windtalker @leiysky @ti-srebot @hanfei1991