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.

Sign up for GitHub

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

Jump to bottom

util/misc: pass the array field in column info to TiKV #46718

Closed
wants to merge 1 commit into from
Closed

util/misc: pass the array field in column info to TiKV #46718

wants to merge 1 commit into from

Conversation

YangKeao
Copy link
Member

@YangKeao YangKeao commented Sep 6, 2023 •
edited

What problem does this PR solve?

Issue Number: close #46717

This PR copies the information of whether the type is an array from the FieldType to the tipb.ColumnInfo and passed it to tikv.

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

@ti-chi-bot
Copy link

ti-chi-bot bot commented Sep 6, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot bot added do-not-merge/needs-triage-completed do-not-merge/needs-tests-checked do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 6, 2023
@tiprow
Copy link

tiprow bot commented Sep 6, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

This was referenced Sep 6, 2023
Closed
Merged
@YangKeao YangKeao marked this pull request as ready for review September 12, 2023 04:56
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2023
@ti-chi-bot ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 12, 2023
@ti-chi-bot ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 12, 2023
@hawkingrei
Copy link
Member

hawkingrei commented Sep 12, 2023 •
edited

/test all

I have uploaded the new dependency.

@YangKeao YangKeao force-pushed the fix-15531 branch 2 times, most recently from c21f8ed to 12789b0 Compare September 12, 2023 05:47
@ti-chi-bot ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 12, 2023
@codecov
Copy link

codecov bot commented Sep 12, 2023 •
edited

Codecov Report

Merging #46718 (6950704) into master (addb06f) will decrease coverage by 0.7153%.
Report is 195 commits behind head on master.
The diff coverage is 100.0000%.

Additional details and impacted files 
@@               Coverage Diff                @@
##             master     #46718        +/-   ##
================================================
- Coverage   73.3691%   72.6539%   -0.7153%     
================================================
  Files          1302       1354        +52     
  Lines        394509     404314      +9805     
================================================
+ Hits         289448     293750      +4302     
- Misses        86664      91920      +5256     
- Partials      18397      18644       +247
Flag Coverage Δ
integration 28.7597% <100.0000%> (?)
unit 73.2406% <100.0000%> (-0.1286%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 53.9913% <ø> (-0.0532%) ⬇️
parser 84.9911% <ø> (-0.0727%) ⬇️
br 48.5852% <ø> (-3.7778%) ⬇️

@ti-chi-bot ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 12, 2023
@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 13, 2023
@YangKeao
Copy link
Member Author

/retest

@YangKeao YangKeao force-pushed the fix-15531 branch 2 times, most recently from 21e752e to d58bb0b Compare September 14, 2023 09:10
@YangKeao
Copy link
Member Author

/retest

bb7133
bb7133 approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member

@bb7133 bb7133 left a comment

Choose a reason for hiding this comment

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

LGTM

However, the title & description is a little misleading: it adds array type to tipb.ColumnInfo and pass it to TiKV, the ColumnInfo struct in TiDB contains array type already, please update the title & desc.

@ti-chi-bot
Copy link

ti-chi-bot bot commented Sep 14, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bb7133

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot
Copy link

ti-chi-bot bot commented Sep 14, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-09-14 10:54:55.753876853 +0000 UTC m=+169261.721464904: ☑️ agreed by bb7133.

@YangKeao YangKeao changed the title util/misc: add array type to column info util/misc: pass the array field in column info to TiKV Sep 14, 2023
YangKeao
YangKeao commented Sep 14, 2023
View reviewed changes
go.mod Outdated
@@ -305,4 +305,6 @@ replace (
github.com/dgrijalva/jwt-go => github.com/form3tech-oss/jwt-go v3.2.6-0.20210809144907-32ab6a8243d7+incompatible
github.com/go-ldap/ldap/v3 => github.com/YangKeao/ldap/v3 v3.4.5-0.20230421065457-369a3bab1117
github.com/pingcap/tidb/parser => ./parser

github.com/pingcap/tipb => github.com/YangKeao/tipb v0.0.0-20230906072756-8d43143257bf
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll hold this PR until pingcap/tipb#317 is merged.

@YangKeao
Copy link
Member Author

/hold

@ti-chi-bot ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2023
@YangKeao
Copy link
Member Author

/unhold

@ti-chi-bot ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2023
@YangKeao
add array type to column info
6950704 
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
@YangKeao
Copy link
Member Author

/retest

1 similar comment
@YangKeao
Copy link
Member Author

/retest

@tiprow
Copy link

tiprow bot commented Sep 14, 2023

@YangKeao: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
tiprow_fast_test 6950704 link true /test tiprow_fast_test

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@ti-chi-bot ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2023
@YangKeao YangKeao mentioned this pull request Sep 14, 2023
Merged
4 tasks
@YangKeao
Copy link
Member Author

/hold

I proposed another solution with less modification and is more suitable with the original multi-valued index implementation. #46993 PTAL @bb7133

I'll also revert the tipb's PR if we agree #46993 is better.

@YangKeao
Copy link
Member Author

#46993 has been merged. Close this one.

@YangKeao YangKeao closed this Sep 25, 2023
@YangKeao YangKeao mentioned this pull request Sep 25, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bb7133 bb7133 bb7133 approved these changes

Assignees
No one assigned
Labels
approved do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-1-more-lgtm release-note-none size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Array field should be passed to the columnInfo
3 participants
@YangKeao @hawkingrei @bb7133