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.

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

rowcodec: make rowcodec can be used for mocktikv & unistore #13774

Merged
merged 7 commits into from
Dec 9, 2019
Merged

rowcodec: make rowcodec can be used for mocktikv & unistore #13774

merged 7 commits into from
Dec 9, 2019

Conversation

lysu
Copy link
Contributor

@lysu lysu commented Nov 27, 2019

What problem does this PR solve?

first part of split PRs of #12634 (there are too many lines but almost them are test)

in #7597, we add rowcodec lib, but after see real-world usage old code need some change, and in this PR add much Unit Test to cover normal code path.

rowcodec provide:

  1. encode:
  • Encode: datumn array into row bytes, pay attention this method will flatten type and make int/unit as non-varint encode.
  1. decoder:
  • DecodeToDatumMap: decode row into datum map, mainly used in ddl operation in TiDB
  • DecodeToChunk: decode row into chunk, mainly used in unistore executor and batch-get/point executor
  • DecodeToBytes: decode row into old datum bytes, mainly used in TiDB memtable reader and mocktikv executor

DecodeToBytes can force varin via WithVarint option to decode int value as varint(in col value, pk value and default value), this will be used in mocktikv analyze columns executor to build table reader .

What is changed and how it works?

  • add new method and let method in they should be

Check List

Tests

Code changes

  • change lib and will be used in next pr

Side effects

  • n/a

Related changes

  • n/a

Release note

  • n/a

This change is Reviewable

@lysu lysu added type/enhancement The issue or PR belongs to an enhancement. component/server labels Nov 27, 2019
@lysu lysu requested review from coocood and jackysp November 27, 2019 09:05
@lysu
Copy link
Contributor Author

lysu commented Nov 27, 2019

/run-all-tests

@codecov
Copy link

codecov bot commented Nov 27, 2019

Codecov Report

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

@@             Coverage Diff             @@
##             master     #13774   +/-   ##
===========================================
  Coverage   80.2253%   80.2253%           
===========================================
  Files           481        481           
  Lines        120791     120791           
===========================================
  Hits          96905      96905           
  Misses        16171      16171           
  Partials       7715       7715

util/rowcodec/decoder.go Outdated Show resolved Hide resolved
util/rowcodec/row.go Outdated Show resolved Hide resolved
util/rowcodec/decoder.go Outdated Show resolved Hide resolved
util/rowcodec/decoder.go Outdated Show resolved Hide resolved
util/rowcodec/encoder.go Outdated Show resolved Hide resolved
util/rowcodec/decoder.go Outdated Show resolved Hide resolved
util/rowcodec/decoder.go Outdated Show resolved Hide resolved
@lysu
Copy link
Contributor Author

lysu commented Dec 2, 2019

@jackysp @coocood I have make some refactor:

mainly split Decoder into DatumMapDecoder/ChunkDecoder/BytesDecoder, after refactor:

  • decode chunk we can provider datum as default directly(avoid datum -> bytes -> datum cost)
  • decode map we can avoid prepare default value, because decode map fill default value in logic in higher layer code
  • decode bytes, we can provider bytes as default directly(avoid bytes -> datum -> bytes cost)

PTAL if free

"github.com/pingcap/tidb/types"
"github.com/pingcap/tidb/types/json"
"github.com/pingcap/tidb/util/chunk"
"github.com/pingcap/tidb/util/codec"
)

// Decoder decodes the row to chunk.Chunk.
// Decoder contains base util for decode row.
type Decoder struct {
Copy link
Member

Choose a reason for hiding this comment

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

can we make it unexported?

func (decoder *Decoder) decodeColData(colIdx int, colData []byte, chk *chunk.Chunk) error {
ft := decoder.requestTypes[colIdx]
switch ft.Tp {
func (decoder *ChunkDecoder) decodeColChunk(colIdx int, col *ColInfo, colData []byte, chk *chunk.Chunk) error {
Copy link
Member

Choose a reason for hiding this comment

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

decodeColToChunk

}

// NewByteDecoder creates a BytesDecoder.
func NewByteDecoder(columns []ColInfo, handleColID int64, defBytes []func() ([]byte, error), loc *time.Location) *BytesDecoder {
Copy link
Member

Choose a reason for hiding this comment

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

We need comments about the defBytes argument.

// BytesDecoder decodes the row to old datums bytes.
type BytesDecoder struct {
decoder
defBytes []func() ([]byte, error)
Copy link
Member

Choose a reason for hiding this comment

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

If we make the defBytes accept an i argument, then we only need to implement one function.

@coocood
Copy link
Member

coocood commented Dec 3, 2019

LGTM

@lysu lysu added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 3, 2019
@lysu
Copy link
Contributor Author

lysu commented Dec 5, 2019

PTAL @jackysp 😄

@lysu
Copy link
Contributor Author

lysu commented Dec 9, 2019

PTAL @jackysp

Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@jackysp jackysp added the status/can-merge Indicates a PR has been approved by a committer. label Dec 9, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Dec 9, 2019

/run-all-tests

@sre-bot sre-bot merged commit b364b89 into pingcap:master Dec 9, 2019
XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/server status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants