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

discard the extended checksum part in row values #739

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zyguan
Copy link

@zyguan zyguan commented Apr 24, 2023

What problem does this PR solve?

Issue Number: ref pingcap/tidb#42747

Problem Description:

Row values now may have extended checksums since pingcap/tidb#42859, so client-java need to discard these checksums on read.

What is changed and how does it work?

Do not copy the checksum part to RowV2.data. Actually the current implementation is ok since RowV2.data can be only accessed by RowV2.getData. This PR just makes it more safer.

Signed-off-by: zyguan <zhongyangguan@gmail.com>
@tidb-cloud-branching-dev
Copy link

tidb-cloud-branching-dev bot commented Apr 24, 2023

The latest updates on TiDB Cloud Branch. Learn more about TiDB Cloud ↗︎

BranchName ClusterID Status Updated
discard-row-checksum_739_5b77567 3059033 running 2023-04-24T06:34:36Z

@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.18 ⚠️

Comparison is base (cb26d58) 37.93% compared to head (eecfdcf) 37.76%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #739      +/-   ##
============================================
- Coverage     37.93%   37.76%   -0.18%     
+ Complexity     1613     1603      -10     
============================================
  Files           278      278              
  Lines         17517    17522       +5     
  Branches       1992     1994       +2     
============================================
- Hits           6645     6617      -28     
- Misses        10208    10237      +29     
- Partials        664      668       +4     
Impacted Files Coverage Δ
src/main/java/org/tikv/common/codec/RowV2.java 0.00% <0.00%> (ø)

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

shiyuhang0
shiyuhang0 previously approved these changes May 31, 2023
@shiyuhang0
Copy link
Collaborator

shiyuhang0 commented May 31, 2023

roughly approve.
But will client-java return row with checksums col before this optimize?
checksum seems a feature of tidb and it does not exist in TiKV, which means it may not influence client-java. have you tested this pr locally?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants