expression: fix wrong flen from baseFunctionClass #56607

joechenrh · 2024-10-12T06:39:54Z

What problem does this PR solve?

Issue Number: close #56587

Problem Summary:

What changed and how does it work?

Previously, quoteFunctionClass may return type with flen = 0 when the underlying argument has the UnspecifiedLength which equals to -1.

tidb/pkg/expression/builtin_string.go

Line 2899 in f0b3bf0

bf.tp.SetFlen(2*args[0].GetType(ctx.GetEvalCtx()).GetFlen() + 2)

And this type will be passed into LogicalUnionAll and a wrong type is deduced.

tidb/pkg/planner/core/logical_plan_builder.go

Lines 1586 to 1591 in f0b3bf0

    
           resultTp := col.RetType 
        
           for j := 1; j < len(u.Children()); j++ { 
        
           	tmpExprs = append(tmpExprs, u.Children()[j].Schema().Columns[i]) 
        
           	childTp := u.Children()[j].Schema().Columns[i].RetType 
        
           	resultTp = unionJoinFieldType(resultTp, childTp) 
        
           }

Some other functions also have this problem, for example:

mysql> select elt(2, "123", cast('abcdefg' as char)) union all select "1234";
+----------------------------------------+
| elt(2, "123", cast('abcdefg' as char)) |
+----------------------------------------+
| abcd                                   |
| 1234                                   |
+----------------------------------------+
2 rows in set, 1 warning (0.00 sec)

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

tiprow · 2024-10-12T06:40:17Z

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

codecov · 2024-10-12T06:52:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.8608%. Comparing base (0e3efb6) to head (3afd694).
Report is 8 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #56607        +/-   ##
================================================
+ Coverage   72.8325%   74.8608%   +2.0283%     
================================================
  Files          1672       1717        +45     
  Lines        462833     472049      +9216     
================================================
+ Hits         337093     353380     +16287     
+ Misses       104947      96604      -8343     
- Partials      20793      22065      +1272

Flag	Coverage Δ
integration	`49.1691% <83.3333%> (?)`
unit	`72.4300% <100.0000%> (+0.2216%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`60.7186% <ø> (+15.6309%)`	⬆️

---- 🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests
JS Bundle Analysis - Avoid shipping oversized bundles

joechenrh · 2024-10-12T08:16:23Z

/retest

tiprow · 2024-10-12T08:16:44Z

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

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

windtalker · 2024-10-15T06:30:44Z

pkg/expression/builtin_string.go

 		if err != nil {
 			return nil, err
 		}
 		argFieldTp := args[0].GetType(ctx.GetEvalCtx())
 		// Use UTF8MB4 as default.
+		if argLen != types.UnspecifiedLength {
+			bf.tp.SetFlen(argLen * 4 * 2)
+		}
 		bf.tp.SetFlen(argFieldTp.GetFlen() * 4 * 2)


This line need to be removed?

windtalker · 2024-10-15T06:34:37Z

pkg/expression/builtin_string.go

 		if err != nil {
 			return nil, err
 		}
 		argFieldTp := args[0].GetType(ctx.GetEvalCtx())
 		// Use UTF8MB4 as default.
+		if argLen != types.UnspecifiedLength {


I think it's better to use argFieldTp.GetFlen() as the argLen because inside newBaseBuiltinFuncWithTp, args[0] maybe changed by wrap some implict cast function

windtalker · 2024-10-15T06:35:16Z

pkg/expression/builtin_string.go

 		if err != nil {
 			return nil, err
 		}
-		bf.tp.SetFlen(args[0].GetType(ctx.GetEvalCtx()).GetFlen() * 2)
+		if argLen != types.UnspecifiedLength {
+			bf.tp.SetFlen(argLen * 2)


Ditto, better to use argFieldTp.GetFlen() as the argLen

windtalker

lgtm

yibin87 · 2024-11-19T07:48:08Z

/test check-dev

tiprow · 2024-11-19T07:48:31Z

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

In response to this:

/test check-dev

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.

yibin87

LGTM

ti-chi-bot · 2024-11-19T07:51:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: windtalker, yibin87

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:

~~OWNERS~~ [windtalker]
~~pkg/expression/OWNERS~~ [windtalker]

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 · 2024-11-19T07:51:18Z

[LGTM Timeline notifier]

Timeline:

2024-10-21 05:46:33.878196916 +0000 UTC m=+243594.574987550: ☑️ agreed by windtalker.
2024-11-19 07:51:17.919284155 +0000 UTC m=+947440.110153151: ☑️ agreed by yibin87.

joechenrh · 2024-11-19T07:55:51Z

/retest

tiprow · 2024-11-19T07:56:12Z

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

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

joechenrh · 2024-11-19T08:25:17Z

/retest

tiprow · 2024-11-19T08:25:39Z

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

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

joechenrh · 2024-11-19T08:31:07Z

/ok-to-test

joechenrh · 2024-11-19T09:32:31Z

/retest

joechenrh · 2024-11-19T10:18:41Z

/retest

ti-chi-bot · 2024-11-19T12:36:12Z

In response to a cherrypick label: new pull request created to branch release-8.5: #57518.

close #56587

Fix flen

cfae08a

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 12, 2024

joechenrh changed the title ~~expression: fix wrong flen from quoteFunctionClass~~ expression: fix wrong flen from baseFunctionClass Oct 12, 2024

Update elt

37e7e44

ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 12, 2024

Update hex

e7da03d

windtalker reviewed Oct 15, 2024

View reviewed changes

Address comment

5837855

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. do-not-merge/needs-triage-completed labels Oct 17, 2024

windtalker approved these changes Oct 21, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Oct 21, 2024

yibin87 approved these changes Nov 19, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 19, 2024

Merge branch 'master' into fix-quote-function

1634a1d

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Nov 19, 2024

Merge branch 'master' into fix-quote-function

3afd694

ti-chi-bot bot merged commit 60bbccb into pingcap:master Nov 19, 2024
24 checks passed

ti-chi-bot bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Nov 19, 2024

ti-chi-bot mentioned this pull request Nov 19, 2024

expression: fix wrong flen from baseFunctionClass (#56607) #57518

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Nov 19, 2024

expression: fix wrong flen from baseFunctionClass (#56607) (#57518)

cb74115

close #56587

joechenrh mentioned this pull request Dec 4, 2024

executor: fix flaky test TestUnionIssue #57972

Merged

13 tasks

joechenrh deleted the fix-quote-function branch December 18, 2024 01:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: fix wrong flen from baseFunctionClass #56607

expression: fix wrong flen from baseFunctionClass #56607

joechenrh commented Oct 12, 2024 •

edited

Loading

tiprow bot commented Oct 12, 2024

codecov bot commented Oct 12, 2024 •

edited

Loading

joechenrh commented Oct 12, 2024

tiprow bot commented Oct 12, 2024

windtalker Oct 15, 2024

windtalker Oct 15, 2024

windtalker Oct 15, 2024

windtalker left a comment

yibin87 commented Nov 19, 2024

tiprow bot commented Nov 19, 2024

yibin87 left a comment

ti-chi-bot bot commented Nov 19, 2024

ti-chi-bot bot commented Nov 19, 2024

joechenrh commented Nov 19, 2024

tiprow bot commented Nov 19, 2024

joechenrh commented Nov 19, 2024

tiprow bot commented Nov 19, 2024

joechenrh commented Nov 19, 2024

joechenrh commented Nov 19, 2024

joechenrh commented Nov 19, 2024

ti-chi-bot commented Nov 19, 2024

	resultTp := col.RetType
	for j := 1; j < len(u.Children()); j++ {
	tmpExprs = append(tmpExprs, u.Children()[j].Schema().Columns[i])
	childTp := u.Children()[j].Schema().Columns[i].RetType
	resultTp = unionJoinFieldType(resultTp, childTp)
	}

expression: fix wrong flen from baseFunctionClass #56607

expression: fix wrong flen from baseFunctionClass #56607

Conversation

joechenrh commented Oct 12, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Oct 12, 2024

codecov bot commented Oct 12, 2024 • edited Loading

Codecov Report

joechenrh commented Oct 12, 2024

tiprow bot commented Oct 12, 2024

windtalker Oct 15, 2024

Choose a reason for hiding this comment

windtalker Oct 15, 2024

Choose a reason for hiding this comment

windtalker Oct 15, 2024

Choose a reason for hiding this comment

windtalker left a comment

Choose a reason for hiding this comment

yibin87 commented Nov 19, 2024

tiprow bot commented Nov 19, 2024

yibin87 left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Nov 19, 2024

ti-chi-bot bot commented Nov 19, 2024

[LGTM Timeline notifier]

joechenrh commented Nov 19, 2024

tiprow bot commented Nov 19, 2024

joechenrh commented Nov 19, 2024

tiprow bot commented Nov 19, 2024

joechenrh commented Nov 19, 2024

joechenrh commented Nov 19, 2024

joechenrh commented Nov 19, 2024

ti-chi-bot commented Nov 19, 2024

joechenrh commented Oct 12, 2024 •

edited

Loading

codecov bot commented Oct 12, 2024 •

edited

Loading