Skip to content

executor: refactor some methods of Row and Chunk#11380

Merged
qw4990 merged 4 commits into
pingcap:masterfrom
qw4990:vecexpr-row-refactor
Jul 23, 2019
Merged

executor: refactor some methods of Row and Chunk#11380
qw4990 merged 4 commits into
pingcap:masterfrom
qw4990:vecexpr-row-refactor

Conversation

@qw4990
Copy link
Copy Markdown
Contributor

@qw4990 qw4990 commented Jul 23, 2019

What problem does this PR solve?

Refactor some code of Row, which is prepared for introducing Sel to Chunk.

What is changed and how it works?

  1. hide some unsafe methods of Chunk which only used in tests;
  2. refactor Row to use vectorized Column API;
  3. add some methods for Column;

All changes are refactoring or new methods, so it's easy to review.

Check List

Tests

  • Unit test

@qw4990 qw4990 added the sig/execution SIG execution label Jul 23, 2019
@qw4990 qw4990 requested review from XuHuaiyu, coocood and zz-jason July 23, 2019 06:26
@qw4990 qw4990 changed the title executor: refactor some codes of Row executor: refactor Row to let it use vectorized Column API Jul 23, 2019
@qw4990 qw4990 force-pushed the vecexpr-row-refactor branch from 696ef19 to dd4e852 Compare July 23, 2019 06:28
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

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.

how about only returning a reference of MyDecimal?

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, and it returns a pointer now.

@zz-jason
Copy link
Copy Markdown
Member

/run-all-tests

Comment thread util/chunk/row.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.

Can we change GetDecimal to return pointer?

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.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 23, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@37bb4ba). Click here to learn what that means.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master     #11380   +/-   ##
===========================================
  Coverage          ?   81.2914%           
===========================================
  Files             ?        423           
  Lines             ?      90392           
  Branches          ?          0           
===========================================
  Hits              ?      73481           
  Misses            ?      11602           
  Partials          ?       5309

@qw4990 qw4990 force-pushed the vecexpr-row-refactor branch from b7235fa to 0b7ce59 Compare July 23, 2019 06:35
@coocood
Copy link
Copy Markdown
Member

coocood commented Jul 23, 2019

LGTM

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 the status/LGT2 Indicates that a PR has LGTM 2. label Jul 23, 2019
@qw4990 qw4990 changed the title executor: refactor Row to let it use vectorized Column API executor: refactor some methods of Row and Chunk Jul 23, 2019
@qw4990 qw4990 merged commit e1c6dfa into pingcap:master Jul 23, 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants