Skip to content

expression, util: add an interface Times for chunk.Column#11742

Merged
qw4990 merged 9 commits into
pingcap:masterfrom
XuHuaiyu:time_vec
Aug 15, 2019
Merged

expression, util: add an interface Times for chunk.Column#11742
qw4990 merged 9 commits into
pingcap:masterfrom
XuHuaiyu:time_vec

Conversation

@XuHuaiyu
Copy link
Copy Markdown
Contributor

@XuHuaiyu XuHuaiyu commented Aug 14, 2019

What problem does this PR solve?

  1. Add an interface chunk.Column.Times
  2. Add an interface chunk.Column.ResizeTime

What is changed and how it works?

Benchmark result

$ GO111MODULE=on go test -bench=BenchmarkTime -run=none 
goos: darwin
goarch: amd64
pkg: github.com/pingcap/tidb/util/chunk
BenchmarkTimeRow-4         10000            151708 ns/op
BenchmarkTimeVec-4         50000             33028 ns/op
PASS

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

Side effects

N/A

Related changes

N/A

@XuHuaiyu XuHuaiyu added type/enhancement The issue or PR belongs to an enhancement. sig/execution SIG execution labels Aug 14, 2019
@XuHuaiyu XuHuaiyu requested a review from qw4990 August 14, 2019 13:25
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 14, 2019

Codecov Report

Merging #11742 into master will decrease coverage by 0.4769%.
The diff coverage is 87.5%.

@@               Coverage Diff               @@
##             master     #11742       +/-   ##
===============================================
- Coverage   81.9352%   81.4583%   -0.477%     
===============================================
  Files           433        433               
  Lines         95274      93190     -2084     
===============================================
- Hits          78063      75911     -2152     
- Misses        11778      11853       +75     
+ Partials       5433       5426        -7

@XuHuaiyu XuHuaiyu changed the title Time vec expression, util: add an interface Times for chunk.Column Aug 14, 2019
@qw4990 qw4990 requested review from coocood and zz-jason August 15, 2019 03:31
Copy link
Copy Markdown
Contributor

@qw4990 qw4990 left a comment

Choose a reason for hiding this comment

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

LGTM

@coocood
Copy link
Copy Markdown
Member

coocood commented Aug 15, 2019

How much memory allocation increased when we store the Time struct directly?

@qw4990
Copy link
Copy Markdown
Contributor

qw4990 commented Aug 15, 2019

/run-all-tests

zz-jason
zz-jason previously approved these changes Aug 15, 2019
Copy link
Copy Markdown
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@qw4990
Copy link
Copy Markdown
Contributor

qw4990 commented Aug 15, 2019

How much memory allocation increased when we store the Time struct directly?

13 bytes -> 20 bytes.

@zz-jason
Copy link
Copy Markdown
Member

How much memory allocation increased when we store the Time struct directly?

@XuHuaiyu Also, could you provide some benchmarks on the evaluation of time values?

@XuHuaiyu
Copy link
Copy Markdown
Contributor Author

@zz-jason Added. And the result is added in the description.

@XuHuaiyu XuHuaiyu requested a review from zz-jason August 15, 2019 03:54
@XuHuaiyu
Copy link
Copy Markdown
Contributor Author

@coocood @qw4990
Actually
14 bytes --> 20 bytes

@qw4990
Copy link
Copy Markdown
Contributor

qw4990 commented Aug 15, 2019

/run-all-tests

@qw4990 qw4990 added status/all tests passed status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Aug 15, 2019
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 15, 2019

/run-all-tests

@qw4990 qw4990 merged commit 1b13a2a into pingcap:master Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sig/execution SIG execution status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/enhancement The issue or PR belongs to an enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants