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

function-and-operators: enrich instructions for string functions BIT_LENGTH() and CHAR() #15482 #16087

Merged

Conversation

PitifulPete
Copy link
Contributor

@PitifulPete PitifulPete commented Jan 10, 2024

What have you changed? (mandatory)

Please explain IN DETAIL what the changes are in this PR and why they are needed:

  • Fix (Enrich instructions for string functions BIT_LENGTH() and CHAR() #15482)
  • Describe the BIT_LENGTH() and CHAR() functions
  • Add three examples that illustrate how BIT_LENGTH() works and four examples that illustrate how CHAR() works.
  • Add a blockquote under the second BIT_LENGTH() example to indicate that the example operates under the assumption that a database exist
  • Add a blockquote before "Further Examples" to explain that CHAR() also works with non-ASCII character

Please NOTE that:

  • Do not assume the code is self-evident
  • Do not assume reviewers understand the original issue

What are the type of the changes? (mandatory)

The currently defined types are listed below, please pick one of the types for this PR by removing the others:

  • Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

Please describe the tests that you ran to verify your changes. Have you finished unit tests, integration tests, or manual tests?

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

YES

If there is document change, please file a PR in (docs and docs-cn) and add the PR number here.

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

PitifulPete and others added 4 commits January 10, 2024 14:47
…R_LENGTH() and CHARACTER_LENGTH()

improve content on string functions
Implemented corrections suggested by @dveeden and responded to one comment

Co-authored-by: Daniël van Eeden <[email protected]>
@ti-chi-bot ti-chi-bot bot added contribution This PR is from a community contributor. first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. missing-translation-status This PR does not have translation status info. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 10, 2024
Copy link
Contributor

@dveeden dveeden left a comment

Choose a reason for hiding this comment

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

Note that this might have overlap with #16068

functions-and-operators/string-functions.md Outdated Show resolved Hide resolved
functions-and-operators/string-functions.md Outdated Show resolved Hide resolved
@dveeden
Copy link
Contributor

dveeden commented Jan 10, 2024

From the linters:

[2024-01-10T14:09:46.029Z] functions-and-operators/string-functions.md: this file has manual line breaks in the following lines:
[2024-01-10T14:09:46.029Z] 
[2024-01-10T14:09:46.029Z] MANUAL LINE BREAKS: L128
[2024-01-10T14:09:46.029Z] MANUAL LINE BREAKS: L157
[2024-01-10T14:09:33.056Z] + markdownlint functions-and-operators/string-functions.md
[2024-01-10T14:09:33.314Z] functions-and-operators/string-functions.md: 121: MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```"]
[2024-01-10T14:09:33.314Z] functions-and-operators/string-functions.md: 122: MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```sql"]
[2024-01-10T14:09:33.314Z] functions-and-operators/string-functions.md: 148: MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```"]
[2024-01-10T14:09:33.314Z] functions-and-operators/string-functions.md: 149: MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```sql"]

@qiancai qiancai added 2024-tidb-docs-dash This issue or PR is included in the 2024 TiDB Docs Dash event. translation/doing This PR's assignee is translating this PR. labels Jan 10, 2024
@ti-chi-bot ti-chi-bot bot removed the missing-translation-status This PR does not have translation status info. label Jan 10, 2024
@qiancai qiancai requested a review from yibin87 January 11, 2024 08:20
PitifulPete and others added 6 commits January 11, 2024 14:51
…R_LENGTH() and CHARACTER_LENGTH()

improve content on string functions
Implemented corrections suggested by @dveeden and responded to one comment

Co-authored-by: Daniël van Eeden <[email protected]>
…ions and add new details

add details and examples that modify previous descriptions
…ions and add new details

add details and examples that modify previous descriptions
@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 Jan 11, 2024
@PitifulPete PitifulPete requested a review from dveeden January 11, 2024 14:26
+----------+
| BIT_LENGTH("TiDB") |
+----------+
| 8 (T) + 8 (i) + 8 (D) + 8 (B) = 40 bits |
Copy link
Contributor

Choose a reason for hiding this comment

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

This is mixing actual output with the description.

mysql> SELECT BIT_LENGTH("TiDB");
+--------------------+
| BIT_LENGTH("TiDB") |
+--------------------+
|                 32 |
+--------------------+
1 row in set (0.01 sec)

Also maybe it would be better to say: "8 bits per character × 4 characters = 40 bits"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i actually think the "8 (T) + 8 (i)..." is clearer because of examples that use non-alphanumeric characters

functions-and-operators/string-functions.md Outdated Show resolved Hide resolved
SELECT CustomerName, BIT_LENGTH(CustomerName) AS BitLengthOfName FROM Customers;
| CustomerName|BitLengthOfName |
|-------------|----------------|
| Albert Einstein | 120 bits |
Copy link
Contributor

Choose a reason for hiding this comment

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

This also mixes actual output with description.

mysql> SELECT CustomerName, BIT_LENGTH(CustomerName) AS BitLengthOfName FROM Customers;
+--------------------+-----------------+
| CustomerName       | BitLengthOfName |
+--------------------+-----------------+
| Albert Einstein    |             120 |
| Robert Oppenheimer |             144 |
+--------------------+-----------------+
2 rows in set (0.01 sec)

To get something similar:

mysql> SELECT CustomerName, FORMAT_BYTES(BIT_LENGTH(CustomerName)*8) AS BitLengthOfName FROM Customers;
+--------------------+-----------------+
| CustomerName       | BitLengthOfName |
+--------------------+-----------------+
| Albert Einstein    | 960 bytes       |
| Robert Oppenheimer | 1.12 KiB        |
+--------------------+-----------------+
2 rows in set (0.00 sec)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please clarify what you mean by "mixing actual output with description"

Copy link
Contributor

Choose a reason for hiding this comment

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

The actual output should be:
+--------------------+
| BIT_LENGTH("TiDB") |
+--------------------+
| 32 |
+--------------------+
Instead of
+----------+
| BIT_LENGTH("TiDB") |
+----------+
| 8 (T) + 8 (i) + 8 (D) + 8 (B) = 32 bits |
+----------+
"8 (T) + 8 (i) + 8 (D) + 8 (B) = 32 bits" is explanation not actual query result.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, got it. thank you @yibin87

functions-and-operators/string-functions.md Outdated Show resolved Hide resolved
functions-and-operators/string-functions.md Outdated Show resolved Hide resolved
Copy link
Contributor

@yibin87 yibin87 left a comment

Choose a reason for hiding this comment

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

There seems multiple commits in this PR: br-related, bit_length, char_length, tidb-global-sort and some others...

SELECT CustomerName, BIT_LENGTH(CustomerName) AS BitLengthOfName FROM Customers;
| CustomerName|BitLengthOfName |
|-------------|----------------|
| Albert Einstein | 120 bits |
Copy link
Contributor

Choose a reason for hiding this comment

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

The actual output should be:
+--------------------+
| BIT_LENGTH("TiDB") |
+--------------------+
| 32 |
+--------------------+
Instead of
+----------+
| BIT_LENGTH("TiDB") |
+----------+
| 8 (T) + 8 (i) + 8 (D) + 8 (B) = 32 bits |
+----------+
"8 (T) + 8 (i) + 8 (D) + 8 (B) = 32 bits" is explanation not actual query result.

| BIT_LENGTH("PingCap 123") |
+----------+
| 8 (P) + 8 (i) + 8 (n) + 8 (g) + 8 (C) + 8 (a) + 8 (p) + 8 () + 8 (1) + 8 (2) + 8 (3) = 88 bits |
+----------+
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto


> **Note:**
>
> The second example operates under the assumption that there is a database with a record titled `Customers` and a field inside titled `CustomerName`
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if word "record" is accurate, in my understanding, it is a table names "Customers" with "CustomerName" column.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm sure it is accurate. record (row) and field (column). admittedly, could have just used column and row tho

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed record to row, and field to column for simplicity, @yibin87

@PitifulPete
Copy link
Contributor Author

There seems multiple commits in this PR: br-related, bit_length, char_length, tidb-global-sort and some others...

yes. not sure why that happened tho

@yibin87
Copy link
Contributor

yibin87 commented Jan 12, 2024

There seems multiple commits in this PR: br-related, bit_length, char_length, tidb-global-sort and some others...

yes. not sure why that happened tho

Maybe you need to create mutilple different branches, all rebase from master, then one branch for one commit, and submit pr from different branches.

@qiancai qiancai self-assigned this Jan 29, 2024
qiancai added a commit to PitifulPete/tidb-docs that referenced this pull request Jan 29, 2024
Copy link
Contributor

@yibin87 yibin87 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

ti-chi-bot bot commented Jan 29, 2024

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

In response to this:

LGTM

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.

@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 29, 2024
Copy link

ti-chi-bot bot commented Jan 29, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-01-29 06:54:04.062508585 +0000 UTC m=+1375685.626806288: ☑️ agreed by dveeden.
  • 2024-01-29 07:38:09.078055318 +0000 UTC m=+1378330.642353022: ☑️ agreed by hfxsd.

@qiancai
Copy link
Collaborator

qiancai commented Jan 30, 2024

@PitifulPete do you need any additional help with this PR? It'd be great to get this merged by February 1 so that you also get points for a merged PR 😄

Hey @rpaik, I actually do not know why it hasn't been merged. Doesn't seem like there is any outstanding issue raised by any of the reviewers that i'm yet to attend to. Hey @qiancai, why haven't this PR and 16057 been merged?

Hi @PitifulPete, that was because some content in this PR was overlapped with #16057. I've cleaned them up in 32dc80e. Now this PR is ready for merge.

@qiancai
Copy link
Collaborator

qiancai commented Jan 30, 2024

/approve

Copy link

ti-chi-bot bot commented Jan 30, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiancai

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 ti-chi-bot bot added the approved label Jan 30, 2024
@qiancai qiancai added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 30, 2024
@qiancai
Copy link
Collaborator

qiancai commented Jan 30, 2024

/retest

@qiancai qiancai removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved labels Jan 30, 2024
@ti-chi-bot ti-chi-bot bot added the approved label Jan 30, 2024
@ti-chi-bot ti-chi-bot bot merged commit f1b3e2b into pingcap:master Jan 30, 2024
9 checks passed
@PitifulPete PitifulPete deleted the enrich/instructions-bitlength-char branch January 30, 2024 08:36
@qiancai qiancai linked an issue Jan 31, 2024 that may be closed by this pull request
@qiancai qiancai 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 Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2024-tidb-docs-dash This issue or PR is included in the 2024 TiDB Docs Dash event. approved area/develop This PR relates to the area of TiDB App development. contribution This PR is from a community contributor. first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. lgtm size/L Denotes a PR that changes 100-499 lines, ignoring generated files. translation/done This PR has been translated from English into Chinese and updated to pingcap/docs-cn in a PR.
Development

Successfully merging this pull request may close these issues.

Enrich instructions for string functions BIT_LENGTH() and CHAR()