Bulk update integer display width by dveeden · Pull Request #19393 · pingcap/docs

dveeden · 2024-11-13T12:25:06Z

What is changed, added or deleted? (Required)

Related to:

What this does is basically the following, with some manual changes:

git grep -il '[^a-z]int([0-9]*)' | grep -vE '^(information-schema|releases)' | xargs sed -i 's/[^a-z]int([0-9]*)/ int/gi'
git grep -il '[^a-z]tinyint([0-9]*)' | grep -vE '^(information-schema|releases)' | xargs sed -i 's/[^a-z]tinyint([0-9]*)/ tinyint/gi'
git grep -il '[^a-z]smallint([0-9]*)' | grep -vE '^(information-schema|releases)' | xargs sed -i 's/[^a-z]smallint([0-9]*)/ smallint/gi'
git grep -il '[^a-z]mediumint([0-9]*)' | grep -vE '^(information-schema|releases)' | xargs sed -i 's/[^a-z]mediumint([0-9]*)/ mediumint/gi'
git grep -il '[^a-z]bigint([0-9]*)' | grep -vE '^(information-schema|releases)' | xargs sed -i 's/[^a-z]bigint([0-9]*)/ bigint/gi'

Things to consider:

This should not affect releases/
We should be careful to not break formatting of table based output
We should be careful to not break examples that rely on display width
The display width may be visible in information_schema information. Also this has a lot of table based output that we should be careful not to break the formatting of.
Display width can still be in (older)MySQL and MariaDB output (for migration info etc)
tinyint(1) is a special case as it is an alias for bool (see data-type-numeric.md)
non-sql should not be replaced (e.g. decimal.NewFromInt(10000))
This should not affect tidb-cloud/

Which TiDB version(s) do your changes apply to? (Required)

Tips for choosing the affected version(s):

By default, CHOOSE MASTER ONLY so your changes will be applied to the next TiDB major or minor releases. If your PR involves a product feature behavior change or a compatibility change, CHOOSE THE AFFECTED RELEASE BRANCH(ES) AND MASTER.

For details, see tips for choosing the affected versions.

What is the related PR or file link(s)?

This PR is translated from:
Other reference link(s):

Do your changes match any of the following descriptions?

Delete files
Change aliases
Need modification after applied to another branch
Might cause conflicts after applied to another branch

ti-chi-bot · 2024-11-13T12:25:09Z

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

mjonss

Just some questions about origin of output, if MySQL/DM or TiDB Cloud/Serverless has the same output and/or version

mjonss · 2024-11-13T13:59:00Z

        {
            "result": true,
-            "msg": "CREATE TABLE `t1` ( `c1` int(11) NOT NULL, `c2` int(11) DEFAULT NULL, PRIMARY KEY (`c1`)) ENGINE=InnoDB DEFAULT CHARSET=latin1 COLLATE=latin1_bin",
+            "msg": "CREATE TABLE `t1` ( `c1` int NOT NULL, `c2` int DEFAULT NULL, PRIMARY KEY (`c1`)) ENGINE=InnoDB DEFAULT CHARSET=latin1 COLLATE=latin1_bin",


Since this is a MySQL Source, should it be updated or not? Is it generated by MySQL, TiDB or DM?

@lance6716 Should we exclude dm/ for now or not?

dm will only save TableInfo and recover the SHOW CREATE TABLE statement using TiDB functions, so it should also be updated

mjonss · 2024-11-13T14:00:43Z

  "schema_name": "db1",
  "table_name": "table1",
-  "schema_create_sql": "CREATE TABLE `t1` (`id` int(11) NOT NULL AUTO_INCREMENT,PRIMARY KEY (`id`) /*T![clustered_index] CLUSTERED */) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin"
+  "schema_create_sql": "CREATE TABLE `t1` (`id` int NOT NULL AUTO_INCREMENT,PRIMARY KEY (`id`) /*T![clustered_index] CLUSTERED */) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin"


Same here, which is the source and where is it generated? MySQL, TiDB or DM?

mjonss · 2024-11-13T14:01:42Z

  -H 'accept: */*' \
  -H 'Content-Type: application/json' \
  -d '{
-  "sql_content": "CREATE TABLE `t1` ( `c1` int(11) DEFAULT NULL, `c2` int(11) DEFAULT NULL, `c3` int(11) DEFAULT NULL) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin;",


Here it is input, so I guess it does not matter, and removing the display width should work regardless.

mjonss · 2024-11-13T14:03:54Z

 ```sql
 CREATE TABLE `test`.`t` (
-    a int(11) PRIMARY KEY,
+    a int PRIMARY KEY,


Similar here I assume MySQL is the upstream, but I have no issues with updating these as well (unless the output is still including the display width from MySQL or DM).

mjonss · 2024-11-13T14:04:26Z

 CREATE TABLE `test`.`t` (
    c_table varchar(10) DEFAULT NULL,
-    a int(11) PRIMARY KEY,
+    a int PRIMARY KEY,


Same here, where does it origin from and will it be with or without display width?

mjonss · 2024-11-13T14:05:49Z

                "owner": "mysql-replica-02",
                "DDLs": [
-                    "USE `shard_db`; ALTER TABLE `shard_db`.`shard_table` ADD COLUMN `c2` int(11);"
+                    "USE `shard_db`; ALTER TABLE `shard_db`.`shard_table` ADD COLUMN `c2` int;"


Is this generated by MySQL or DM? Will it use display width or not?

ti-chi-bot · 2024-11-13T14:14:41Z

@mjonss: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

Just some questions about origin of output, if MySQL/DM or TiDB Cloud/Serverless has the same output and/or version

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.

Oreoxmt

Rest LGTM

Oreoxmt · 2024-11-19T04:21:00Z

/hold

ti-chi-bot · 2024-12-02T02:55:08Z

[LGTM Timeline notifier]

Timeline:

2024-11-19 03:43:51.257957962 +0000 UTC m=+932593.448826961: ☑️ agreed by Oreoxmt.
2024-12-02 02:55:07.226848232 +0000 UTC m=+1037094.846502747: ☑️ agreed by qiancai.

Oreoxmt · 2024-12-02T03:38:03Z

/unhold

Oreoxmt · 2024-12-02T03:38:07Z

/approve

ti-chi-bot · 2024-12-02T03:38:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Oreoxmt

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~~ [Oreoxmt]

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

Bulk update integer display width

b1539b7

dveeden requested review from Oreoxmt, hfxsd and qiancai November 13, 2024 12:25

Various manual fixes

7fe5935

ti-chi-bot Bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 13, 2024

dveeden requested review from mjonss and removed request for hfxsd November 13, 2024 12:39

dveeden marked this pull request as ready for review November 13, 2024 12:39

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 Nov 13, 2024

mjonss approved these changes Nov 13, 2024

View reviewed changes

dveeden requested a review from lance6716 November 13, 2024 19:47

Oreoxmt self-assigned this Nov 14, 2024

Oreoxmt added translation/doing This PR's assignee is translating this PR. for-release This PR relates to a TiDB release but does not have a tracked feature. v8.5 and removed missing-translation-status This PR does not have translation status info. labels Nov 14, 2024

Don't update

38c7754

ti-chi-bot Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 14, 2024

Oreoxmt reviewed Nov 19, 2024

View reviewed changes

Comment thread basic-sql-operations.md Outdated

Oreoxmt approved these changes Nov 19, 2024

View reviewed changes

ti-chi-bot Bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Nov 19, 2024

Oreoxmt mentioned this pull request Nov 19, 2024

Bulk update integer display width pingcap/docs-cn#19121

Merged

16 tasks

Oreoxmt added translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR. and removed translation/doing This PR's assignee is translating this PR. labels Nov 19, 2024

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 Nov 19, 2024

qiancai approved these changes Dec 2, 2024

View reviewed changes

ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 2, 2024

Apply suggestions from code review

d93d922

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 Dec 2, 2024

ti-chi-bot Bot added the approved label Dec 2, 2024

ti-chi-bot Bot merged commit 22341b8 into pingcap:master Dec 2, 2024

Conversation

dveeden commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is changed, added or deleted? (Required)

Which TiDB version(s) do your changes apply to? (Required)

What is the related PR or file link(s)?

Do your changes match any of the following descriptions?

Uh oh!

ti-chi-bot Bot commented Nov 13, 2024

Uh oh!

mjonss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ti-chi-bot Bot commented Nov 13, 2024

Uh oh!

Uh oh!

Oreoxmt left a comment

Choose a reason for hiding this comment

Uh oh!

Oreoxmt commented Nov 19, 2024

Uh oh!

ti-chi-bot Bot commented Dec 2, 2024

[LGTM Timeline notifier]

Uh oh!

Oreoxmt commented Dec 2, 2024

Uh oh!

Oreoxmt commented Dec 2, 2024

Uh oh!

ti-chi-bot Bot commented Dec 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dveeden commented Nov 13, 2024 •

edited

Loading