expression: Support more locales for func FORMAT() | tidb-test=pr/2626 #64316

ghoshh · 2025-11-06T08:22:48Z

What problem does this PR solve?

Issue Number: ref #56167

Problem Summary:

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

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.

None

…_BD,bn_IN,dz_BT...

ti-chi-bot · 2025-11-06T08:22:58Z

Hi @ghoshh. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

tiprow · 2025-11-06T08:23:06Z

Hi @ghoshh. 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.

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-sigs/prow repository.

lance6716 · 2025-11-06T08:47:52Z

/ok-to-test

lance6716 · 2025-11-06T08:49:28Z

/cc @dveeden

pkg/parser/mysql/locale_format.go

tangenta

Rest LGTM

dveeden

Looks like warnings for incorrect locales are missing.

MySQL:

mysql-9.5.0> \W
Show warnings enabled.
mysql-9.5.0> SELECT FORMAT(1234.56,2,'en_US');
+---------------------------+
| FORMAT(1234.56,2,'en_US') |
+---------------------------+
| 1,234.56                  |
+---------------------------+
1 row in set (0.000 sec)

mysql-9.5.0> SELECT FORMAT(1234.56,2,'en_NL');
+---------------------------+
| FORMAT(1234.56,2,'en_NL') |
+---------------------------+
| 1,234.56                  |
+---------------------------+
1 row in set, 1 warning (0.000 sec)

Warning (Code 1649): Unknown locale: 'en_NL'

TiDB (this PR):

tidb-8.0.11-TiDB-v9.0.0-beta.2.pre-730-g451b09b426> \W
Show warnings enabled.
tidb-8.0.11-TiDB-v9.0.0-beta.2.pre-730-g451b09b426> SELECT FORMAT(1234.56,2,'en_US');
+---------------------------+
| FORMAT(1234.56,2,'en_US') |
+---------------------------+
| 1,234.56                  |
+---------------------------+
1 row in set (0.001 sec)

tidb-8.0.11-TiDB-v9.0.0-beta.2.pre-730-g451b09b426> SELECT FORMAT(1234.56,2,'en_NL');
+---------------------------+
| FORMAT(1234.56,2,'en_NL') |
+---------------------------+
| 1,234.56                  |
+---------------------------+
1 row in set (0.000 sec)

tidb-8.0.11-TiDB-v9.0.0-beta.2.pre-730-g451b09b426>

dveeden · 2025-11-06T09:47:47Z

In #56168 I tried to use golang.org/x/text/{language,message,number} so we can automatically benefit from improvements in the library done by others... but that seems to introduce more issues than it solves.

But that PR might have some useful comments.

dveeden · 2025-11-06T09:50:23Z

A difference between MySQL and TiDB:

mysql-9.5.0> SELECT FORMAT(1234.56,2,'fr_FR');
+---------------------------+
| FORMAT(1234.56,2,'fr_FR') |
+---------------------------+
| 1234,56                   |
+---------------------------+
1 row in set (0.000 sec)

tidb-8.0.11-TiDB-v9.0.0-beta.2.pre-730-g451b09b426> SELECT FORMAT(1234.56,2,'fr_FR');
+---------------------------+
| FORMAT(1234.56,2,'fr_FR') |
+---------------------------+
| 1 234,56                  |
+---------------------------+
1 row in set (0.000 sec)

dveeden · 2025-11-06T09:51:34Z

I would suggest to create a list of locales and then connect to TiDB and MySQL and for each of them compare the output.

dveeden · 2025-11-06T15:49:34Z

For check_dev:

run test [expression/builtin] err: sql:select format(12332.2, 2,'es_EC');: failed to run query 
"select format(12332.2, 2,'es_EC');" 
 around line 935, 
we need(72):
select format(12332.2, 2,'es_EC');
format(12332.2, 2,'es_EC')
12,332.20

but got(72):
select format(12332.2, 2,'es_EC');
format(12332.2, 2,'es_EC')
12.332,20

MySQL does this:

mysql> select format(12332.2, 2,'es_EC');
+----------------------------+
| format(12332.2, 2,'es_EC') |
+----------------------------+
| 12.332,20                  |
+----------------------------+
1 row in set (0.000 sec)

This looks like an issue with the test as the new result matches the MySQL output.

dveeden · 2025-11-06T16:36:25Z

I did this:

cd tests/integrationtest
./run-tests.sh -r expression/builtin
git diff

The result is this:

diff --git a/tests/integrationtest/r/expression/builtin.result b/tests/integrationtest/r/expression/builtin.result
index 38500f943c..59c612eda1 100644
--- a/tests/integrationtest/r/expression/builtin.result
+++ b/tests/integrationtest/r/expression/builtin.result
@@ -2103,9 +2103,7 @@ format(NULL, 4)   format(12332.2, NULL)
 NULL   NULL
 select format(12332.2, 2,'es_EC');
 format(12332.2, 2,'es_EC')
-12,332.20
-Level  Code    Message
-Warning        1649    Unknown locale: 'es_EC'
+12.332,20
 select field(1, 2, 1), field(1, 0, NULL), field(1, NULL, 2, 1), field(NULL, 1, 2, NULL);
 field(1, 2, 1) field(1, 0, NULL)       field(1, NULL, 2, 1)    field(NULL, 1, 2, NULL)
 2      0       3       0

This should fix the check_dev test if you commit it.

dveeden · 2025-11-06T16:44:43Z

The mysql-test does show the case where there is no warning where there should be one for non-existent locales

codecov · 2025-11-06T16:48:28Z

Codecov Report

❌ Patch coverage is 82.35294% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.3558%. Comparing base (779e298) to head (cbb252d).
⚠️ Report is 14 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #64316        +/-   ##
================================================
+ Coverage   72.7432%   73.3558%   +0.6125%     
================================================
  Files          1861       1863         +2     
  Lines        504392     508411      +4019     
================================================
+ Hits         366911     372949      +6038     
+ Misses       115176     113176      -2000     
+ Partials      22305      22286        -19

Flag	Coverage Δ
integration	`41.7885% <42.8571%> (?)`
unit	`72.5562% <70.5882%> (+0.2429%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.8700% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`45.5654% <ø> (-0.7452%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

… locale(not modified builtin_string_vec.go yet)

ti-chi-bot · 2025-11-07T04:24:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign bb7133, xuhuaiyu for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found 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

dveeden · 2025-11-07T06:53:20Z

=== RUN   TestFormatWithLocale
    builtin_string_test.go:2871: 
        	Error Trace:	pkg/expression/builtin_string_test.go:2871
        	Error:      	"[{Warning [expression:1649]Unknown locale: 'de_GE'}]" should have 0 item(s), but has 1
        	Test:       	TestFormatWithLocale
        	Messages:   	test: Invalid locale 'de_GE' fallback
--- FAIL: TestFormatWithLocale (0.02s)

                // Test an invalid/unmapped locale
                // Should fallback to en_US (styleCommaDot) with NO error and NO warning.
                {12345.67, 2, "de_GE", "12,345.67", false, "Invalid locale 'de_GE' fallback"},
                {12345.67, 2, "non_existent", "12,345.67", false, "Invalid locale 'non_existent' fallback"},

For the de_GE locale, which is invalid it should fallback to en_US but then also produce a warning, right?

ghoshh · 2025-11-07T06:58:48Z

=== RUN   TestFormatWithLocale
    builtin_string_test.go:2871: 
        	Error Trace:	pkg/expression/builtin_string_test.go:2871
        	Error:      	"[{Warning [expression:1649]Unknown locale: 'de_GE'}]" should have 0 item(s), but has 1
        	Test:       	TestFormatWithLocale
        	Messages:   	test: Invalid locale 'de_GE' fallback
--- FAIL: TestFormatWithLocale (0.02s)

                // Test an invalid/unmapped locale
                // Should fallback to en_US (styleCommaDot) with NO error and NO warning.
                {12345.67, 2, "de_GE", "12,345.67", false, "Invalid locale 'de_GE' fallback"},
                {12345.67, 2, "non_existent", "12,345.67", false, "Invalid locale 'non_existent' fallback"},

For the de_GE locale, which is invalid it should fallback to en_US but then also produce a warning, right?

Oh，yes，I forgot to change this test case.

dveeden · 2025-11-07T07:20:05Z

I found another issue unfortunately:

MySQL:

mysql-9.5.0> WITH l AS (SELECT 'en_US' ln UNION ALL SELECT 'ru_RU' UNION ALL SELECT 'fr_BE' UNION ALL SELECT 'it_CH') SELECT ln,FORMAT(1234.67,3,ln) FROM l;
+-------+----------------------+
| ln    | FORMAT(1234.67,3,ln) |
+-------+----------------------+
| en_US | 1,234.670            |
| ru_RU | 1 234,670            |
| fr_BE | 1234,670             |
| it_CH | 1'234,670            |
+-------+----------------------+
4 rows in set (0.026 sec)

TiDB (this PR):

tidb> WITH l AS (SELECT 'en_US' ln UNION ALL SELECT 'ru_RU' UNION ALL SELECT 'fr_BE' UNION ALL SELECT 'it_CH') SELECT ln,FORMAT(1234.67,3,ln) FROM l;
+-------+----------------------+
| ln    | FORMAT(1234.67,3,ln) |
+-------+----------------------+
| en_US | 1,234.670            |
| fr_BE | 1,234.670            |
| ru_RU | 1,234.670            |
| it_CH | 1,234.670            |
+-------+----------------------+
4 rows in set, 3 warnings (0.001 sec)

tidb> SHOW WARNINGS;
+---------+------+-------------------------+
| Level   | Code | Message                 |
+---------+------+-------------------------+
| Warning | 1649 | Unknown locale: 'fr_BE' |
| Warning | 1649 | Unknown locale: 'ru_RU' |
| Warning | 1649 | Unknown locale: 'it_CH' |
+---------+------+-------------------------+
3 rows in set (0.001 sec)

WITH l AS (
  SELECT 'en_US' ln 
  UNION ALL 
  SELECT 'ru_RU' 
  UNION ALL 
  SELECT 'fr_BE' 
  UNION ALL 
  SELECT 'it_CH'
)
SELECT ln,FORMAT(1234.67,3,ln) FROM l

ti-chi-bot · 2025-11-07T14:53:00Z

@ghoshh: 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
idc-jenkins-ci-tidb/mysql-test	`cbb252d`	link	true	`/test mysql-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-sigs/prow repository. I understand the commands that are listed here.

Support more locales for FORMAT() except for south asia like as_IN,bn…

451b09b

…_BD,bn_IN,dz_BT...

ti-chi-bot bot added do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. contribution This PR is from a community contributor. and removed do-not-merge/needs-tests-checked labels Nov 6, 2025

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Nov 6, 2025

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Nov 6, 2025

ti-chi-bot bot requested a review from dveeden November 6, 2025 08:49

tangenta reviewed Nov 6, 2025

View reviewed changes

pkg/parser/mysql/locale_format.go Outdated Show resolved Hide resolved

tangenta reviewed Nov 6, 2025

View reviewed changes

pkg/parser/mysql/locale_format.go Outdated Show resolved Hide resolved

tangenta reviewed Nov 6, 2025

View reviewed changes

dveeden reviewed Nov 6, 2025

View reviewed changes

replace WRITE with WriteString to reduce unnecessary allocation

72ca179

ghoshh closed this Nov 6, 2025

ghoshh reopened this Nov 6, 2025

update expression/builtin.result and add warnings when met an unknown…

d7ac0e2

… locale(not modified builtin_string_vec.go yet)

update the test case with unknown locale

c01392a

dveeden changed the title ~~expression: Support more locales for func FORMAT()~~ expression: Support more locales for func FORMAT() | tidb-test=pr/2626 Nov 7, 2025

update builtin_string_vec.go

cbb252d

expression: Support more locales for func FORMAT() | tidb-test=pr/2626 #64316

Are you sure you want to change the base?

expression: Support more locales for func FORMAT() | tidb-test=pr/2626 #64316

Conversation

ghoshh commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

Uh oh!

ti-chi-bot bot commented Nov 6, 2025

Uh oh!

tiprow bot commented Nov 6, 2025

Uh oh!

lance6716 commented Nov 6, 2025

Uh oh!

lance6716 commented Nov 6, 2025

Uh oh!

Uh oh!

Uh oh!

tangenta left a comment

Choose a reason for hiding this comment

Uh oh!

dveeden left a comment

Choose a reason for hiding this comment

Uh oh!

dveeden commented Nov 6, 2025

Uh oh!

dveeden commented Nov 6, 2025

Uh oh!

dveeden commented Nov 6, 2025

Uh oh!

dveeden commented Nov 6, 2025

Uh oh!

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

Uh oh!

Uh oh!

dveeden commented Nov 6, 2025

Uh oh!

codecov bot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ti-chi-bot bot commented Nov 7, 2025

Uh oh!

dveeden commented Nov 7, 2025

Uh oh!

ghoshh commented Nov 7, 2025

Uh oh!

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

Uh oh!

Uh oh!

ti-chi-bot bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ghoshh commented Nov 6, 2025 •

edited

Loading

dveeden commented Nov 6, 2025 •

edited

Loading

codecov bot commented Nov 6, 2025 •

edited

Loading

dveeden commented Nov 7, 2025 •

edited

Loading

ti-chi-bot bot commented Nov 7, 2025 •

edited

Loading