parser,config: Correct handling of display width deprecation except for zerofill and tinyint(1) by dveeden · Pull Request #46651 · pingcap/tidb

dveeden · 2023-09-04T15:46:30Z

What problem does this PR solve?

Issue Number: close #46650

Problem Summary:

What is changed and how it works?

Check List

Tests

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

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Display width deprecation now excludes `tinyint(1)` and types with `ZEROFILL`. This improves compatibility with MySQL Connectors.

tiprow · 2023-09-04T15:46:59Z

Hi @dveeden. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Details

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.

dveeden · 2023-09-04T16:17:46Z

/retest

dveeden · 2023-09-04T16:18:00Z

/check-issue-triage-complete

tiprow · 2023-09-04T16:18:19Z

@dveeden: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

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.

dveeden · 2023-09-04T16:19:23Z

/ok-to-test

dveeden · 2023-09-05T07:36:06Z

I removed the change for the default value of DeprecateIntegerDisplayWidth and only fix the handling of tinyint(1) and zerofill. Then we can change the default later.

codecov · 2023-09-05T07:40:13Z

Codecov Report

Merging #46651 (c91059b) into master (302786f) will decrease coverage by 0.4032%.
Report is 36 commits behind head on master.
The diff coverage is 100.0000%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #46651        +/-   ##
================================================
- Coverage   73.3439%   72.9407%   -0.4032%     
================================================
  Files          1314       1340        +26     
  Lines        395407     405743     +10336     
================================================
+ Hits         290007     295952      +5945     
- Misses        86935      91221      +4286     
- Partials      18465      18570       +105

Flag	Coverage Δ
integration	`27.8484% <ø> (?)`
unit	`73.6031% <100.0000%> (+0.2591%)`	⬆️

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

Components	Coverage Δ
dumpling	`54.0444% <ø> (ø)`
parser	`85.0504% <100.0000%> (+0.0876%)`	⬆️
br	`49.3420% <ø> (-2.9659%)`	⬇️

📢 Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pingcap).

mjonss

LGTM, just a question if we should also have a warning for ZEROFILL being deprecated? (should we create an issue for that or just accept that we are not raising a warning, like MySQL does?)

ti-chi-bot · 2023-09-07T11:53:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mjonss, wjhuang2016

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mjonss,wjhuang2016]

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 · 2023-09-07T11:54:02Z

[LGTM Timeline notifier]

Timeline:

2023-09-07 09:11:00.554081125 +0000 UTC m=+2609425.103097112: ☑️ agreed by mjonss.
2023-09-07 11:54:01.452021629 +0000 UTC m=+2619206.001037615: ☑️ agreed by wjhuang2016.

dveeden · 2023-09-07T14:25:01Z

/retest

dveeden requested review from bb7133, mjonss and wjhuang2016 September 4, 2023 15:46

ti-chi-bot Bot added do-not-merge/needs-triage-completed release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/invalid-title size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 4, 2023

dveeden requested a review from AilinKid September 4, 2023 16:03

dveeden changed the title ~~Deprecate integer display width true~~ Deprecate integer display width, except for zerofill and tinyint(1) Sep 4, 2023

dveeden changed the title ~~Deprecate integer display width, except for zerofill and tinyint(1)~~ parser,config: Deprecate integer display width, except for zerofill and tinyint(1) Sep 4, 2023

ti-chi-bot Bot removed the do-not-merge/invalid-title label Sep 4, 2023

ti-chi-bot Bot removed the do-not-merge/needs-triage-completed label Sep 4, 2023

ti-chi-bot Bot added the ok-to-test Indicates a PR is ready to be tested. label Sep 4, 2023

dveeden requested a review from a team as a code owner September 5, 2023 07:11

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 5, 2023

parser: Exclude tinyint(1) and zerofill from display width deprecation

647522a

dveeden force-pushed the DeprecateIntegerDisplayWidth_true branch from d90cecd to 647522a Compare September 5, 2023 07:29

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 5, 2023

dveeden changed the title ~~parser,config: Deprecate integer display width, except for zerofill and tinyint(1)~~ parser,config: Correct handling of display width deprecation except for zerofill and tinyint(1) Sep 5, 2023

Add test for CompactStr

8e04f59

ti-chi-bot Bot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 5, 2023

ti-chi-bot Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 5, 2023

dveeden added 3 commits September 5, 2023 10:22

Fix nogo/revive issue

118aa49

Fix formatting

1df7eec

Update BUILD.bazel

c91059b

mjonss approved these changes Sep 7, 2023

View reviewed changes

Comment thread executor/test/showtest/show_test.go

ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 7, 2023

dveeden mentioned this pull request Sep 7, 2023

Warning for ZEROFILL #46753

Closed

wjhuang2016 approved these changes Sep 7, 2023

View reviewed changes

ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 7, 2023

ti-chi-bot Bot merged commit 83e3565 into pingcap:master Sep 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parser,config: Correct handling of display width deprecation except for zerofill and tinyint(1)#46651

parser,config: Correct handling of display width deprecation except for zerofill and tinyint(1)#46651
ti-chi-bot[bot] merged 5 commits into
pingcap:masterfrom
dveeden:DeprecateIntegerDisplayWidth_true

dveeden commented Sep 4, 2023 •

edited

Loading

Uh oh!

tiprow Bot commented Sep 4, 2023

Uh oh!

dveeden commented Sep 4, 2023

Uh oh!

dveeden commented Sep 4, 2023

Uh oh!

tiprow Bot commented Sep 4, 2023

Uh oh!

dveeden commented Sep 4, 2023

Uh oh!

dveeden commented Sep 5, 2023 •

edited

Loading

Uh oh!

codecov Bot commented Sep 5, 2023 •

edited

Loading

Uh oh!

mjonss left a comment

Uh oh!

Uh oh!

ti-chi-bot Bot commented Sep 7, 2023

Uh oh!

ti-chi-bot Bot commented Sep 7, 2023

Uh oh!

dveeden commented Sep 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dveeden commented Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

tiprow Bot commented Sep 4, 2023

Uh oh!

dveeden commented Sep 4, 2023

Uh oh!

dveeden commented Sep 4, 2023

Uh oh!

tiprow Bot commented Sep 4, 2023

Uh oh!

dveeden commented Sep 4, 2023

Uh oh!

dveeden commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mjonss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ti-chi-bot Bot commented Sep 7, 2023

Uh oh!

ti-chi-bot Bot commented Sep 7, 2023

[LGTM Timeline notifier]

Uh oh!

dveeden commented Sep 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dveeden commented Sep 4, 2023 •

edited

Loading

dveeden commented Sep 5, 2023 •

edited

Loading

codecov Bot commented Sep 5, 2023 •

edited

Loading