Skip to content

util/rowcodec: add new row codec lib#7597

Merged
coocood merged 9 commits into
pingcap:masterfrom
coocood:row-codec
Aug 12, 2019
Merged

util/rowcodec: add new row codec lib#7597
coocood merged 9 commits into
pingcap:masterfrom
coocood:row-codec

Conversation

@coocood
Copy link
Copy Markdown
Member

@coocood coocood commented Sep 4, 2018

What problem does this PR solve?

Prepare to implement the new row format described in the proposal

What is changed and how it works?

Add the rowcodec library and tests, not used in TiDB yet.

Check List

Tests

  • Unit test

Code changes

  • Has exported function/method change

This change is Reviewable

@jackysp jackysp added the type/enhancement The issue or PR belongs to an enhancement. label Dec 12, 2018
@breezewish
Copy link
Copy Markdown
Member

How is the progress of this PR?

@zz-jason
Copy link
Copy Markdown
Member

zz-jason commented Aug 3, 2019

@coocood friendly ping, any update?

@coocood
Copy link
Copy Markdown
Member Author

coocood commented Aug 5, 2019

@zz-jason
I will refine this PR this week.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 5, 2019

Codecov Report

Merging #7597 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master      #7597   +/-   ##
===========================================
  Coverage   81.4444%   81.4444%           
===========================================
  Files           432        432           
  Lines         93088      93088           
===========================================
  Hits          75815      75815           
  Misses        11857      11857           
  Partials       5416       5416

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.

This PR is so huge. Can you split it into several small PRs?

Comment thread util/rowcodec/common.go Outdated
Comment thread util/rowcodec/common.go Outdated
Comment thread util/rowcodec/decoder.go Outdated
@coocood
Copy link
Copy Markdown
Member Author

coocood commented Aug 5, 2019

@zz-jason
It's expensive to split into multiple PRs because basically there just two functions encode and decode, and we need both of them to run the test.

@zz-jason zz-jason self-requested a review August 5, 2019 12:12
Comment thread util/rowcodec/decoder.go
@jackysp
Copy link
Copy Markdown
Contributor

jackysp commented Aug 7, 2019

Should we add some benchmark results in the description?

Comment thread util/rowcodec/encoder.go
Comment thread util/rowcodec/encoder.go
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

Comment thread util/rowcodec/common.go
Comment thread util/rowcodec/common.go Outdated
Comment thread util/rowcodec/common.go Outdated
Comment thread util/rowcodec/common.go Outdated
Comment thread util/rowcodec/common.go Outdated
Comment thread util/rowcodec/common.go
Comment thread util/rowcodec/common.go
Comment thread util/rowcodec/common.go
Comment thread util/rowcodec/encoder.go Outdated
Comment thread util/rowcodec/common.go
@coocood
Copy link
Copy Markdown
Member Author

coocood commented Aug 9, 2019

@zz-jason PTAL

@coocood coocood added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 12, 2019
Comment thread util/rowcodec/encoder.go
Comment thread util/rowcodec/encoder.go
}
if r.isLarge {
largeNotNullSorter := (*largeNotNullSorter)(encoder)
sort.Sort(largeNotNullSorter)
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.

why do we need to sort colIDs and the values for each row?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So we can do binary searches later.

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

@coocood coocood added status/LGT2 Indicates that a PR has LGTM 2. status/can-merge Indicates a PR has been approved by a committer. and removed status/LGT1 Indicates that a PR has LGTM 1. status/PTAL labels Aug 12, 2019
@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 12, 2019

/run-all-tests

@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 12, 2019

/run-all-tests

@sre-bot
Copy link
Copy Markdown
Contributor

sre-bot commented Aug 12, 2019

@coocood merge failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

6 participants