Skip to content

executor: add reserve methods to Column for var-length types#11699

Merged
zz-jason merged 1 commit into
pingcap:masterfrom
qw4990:vecexpr-resize-reserve
Aug 12, 2019
Merged

executor: add reserve methods to Column for var-length types#11699
zz-jason merged 1 commit into
pingcap:masterfrom
qw4990:vecexpr-resize-reserve

Conversation

@qw4990
Copy link
Copy Markdown
Contributor

@qw4990 qw4990 commented Aug 9, 2019

What problem does this PR solve?

Add reserve methods to Column for var-length types to reserve memory before using a Column.
And rename methods PreAllocXXX to ResizeXXX.

What is changed and how it works?

  1. Add reserve methods for JSON, Enum, Bytes, Set and String;
  2. Rename all PreAllocXXX methods.

Check List

Tests

  • Unit test

@qw4990 qw4990 force-pushed the vecexpr-resize-reserve branch 2 times, most recently from 4c71af1 to 273d52f Compare August 9, 2019 08:23
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 9, 2019

Codecov Report

Merging #11699 into master will decrease coverage by 0.2934%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             master     #11699        +/-   ##
================================================
- Coverage   81.7341%   81.4407%   -0.2935%     
================================================
  Files           430        429         -1     
  Lines         94269      92746      -1523     
================================================
- Hits          77050      75533      -1517     
- Misses        11796      11821        +25     
+ Partials       5423       5392        -31

@qw4990 qw4990 requested review from XuHuaiyu, coocood and zz-jason August 9, 2019 08:30
Comment thread util/chunk/column.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
//reserve makes the column capacity be at least enough to contain n elements.
// reserve makes the column capacity be at least enough to contain n elements.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment thread util/chunk/column.go Outdated
@qw4990 qw4990 force-pushed the vecexpr-resize-reserve branch from 273d52f to 25faea3 Compare August 9, 2019 09:26
Comment thread util/chunk/column.go Outdated
@qw4990 qw4990 force-pushed the vecexpr-resize-reserve branch from 25faea3 to 5d1ad8e Compare August 9, 2019 09:41
@coocood
Copy link
Copy Markdown
Member

coocood commented Aug 9, 2019

LGTM

Copy link
Copy Markdown
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.

The title for this PR should be more specific

Comment thread util/chunk/column.go Outdated
@qw4990 qw4990 force-pushed the vecexpr-resize-reserve branch from 5d1ad8e to 3076f7e Compare August 12, 2019 02:58
zz-jason
zz-jason previously approved these changes Aug 12, 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

@zz-jason zz-jason added status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. labels Aug 12, 2019
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 12, 2019

/run-all-tests

update comment

update comment

update

update comments

add UT

address comments

address comment

add comment
@qw4990 qw4990 force-pushed the vecexpr-resize-reserve branch from fab6a7c to 27913b8 Compare August 12, 2019 06:37
@qw4990 qw4990 removed the status/can-merge Indicates a PR has been approved by a committer. label Aug 12, 2019
@qw4990
Copy link
Copy Markdown
Contributor Author

qw4990 commented Aug 12, 2019

I have rebased this PR to the latest branch Master, so please help me to approve this PR again. @zz-jason @XuHuaiyu
And please don't add the label can merge to this PR since I will merge it manually to avoid wrong merge logs.

@zz-jason zz-jason merged commit 5cbb17c into pingcap:master Aug 12, 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/LGT2 Indicates that a PR has LGTM 2. type/new-feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants