planner, expr: eval readonly user var during plan phase | tidb-test=pr/2419 #54462

winoros · 2024-07-04T21:44:16Z

What problem does this PR solve?

Issue Number: close #52742, #8733, #14231, #24276

Problem Summary:

What changed and how does it work?

If a user var is not affected by setvar at SQL level, we can directly use its value. No need to wrap it with getvar.

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.

Support fold read-only user variables to constants
支持将只读的用户变量折叠为常量

codecov · 2024-07-04T22:06:17Z

Codecov Report

Attention: Patch coverage is 89.06250% with 7 lines in your changes missing coverage. Please review.

Project coverage is 73.6128%. Comparing base (6806af4) to head (8aeca9d).
Report is 13 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #54462        +/-   ##
================================================
+ Coverage   73.5201%   73.6128%   +0.0926%     
================================================
  Files          1681       1681                
  Lines        464346     466462      +2116     
================================================
+ Hits         341388     343376      +1988     
- Misses       102131     102280       +149     
+ Partials      20827      20806        -21

Flag	Coverage Δ
integration	`42.8800% <76.5625%> (?)`
unit	`72.3415% <89.0625%> (+0.0660%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.6910% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`45.6167% <ø> (-0.1548%)`	⬇️

pkg/planner/core/plan_cache.go

ti-chi-bot · 2024-08-30T08:21:46Z

PR needs rebase.

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.

pkg/expression/exprstatic/evalctx.go

Co-authored-by: 王超 <[email protected]>

pkg/planner/core/plan_cache.go

…nto eval-readonly-var

winoros · 2024-11-04T18:45:09Z

/retest

qw4990 · 2024-11-05T03:44:00Z

pkg/planner/core/plan_cache.go

+		var (
+			val types.Datum
+			err error
+		)
+		val, err = usingParam.Eval(sctx.GetExprCtx().GetEvalCtx(), chunk.Row{})


Suggested change

var (

val types.Datum

err error

)

val, err = usingParam.Eval(sctx.GetExprCtx().GetEvalCtx(), chunk.Row{})

val, err := usingParam.Eval(sctx.GetExprCtx().GetEvalCtx(), chunk.Row{})

winoros · 2024-11-05T17:07:29Z

pkg/expression/builtin_other.go

+	d, ok := ctx.GetEvalCtx().GetUserVarsReader().GetUserVarVal(arg0.Value.GetString())
+	if ok && d.Kind() == types.KindBinaryLiteral {
+		v.SetBinaryLiteral(v.GetBytes())
+	}


Maybe we need a reuse here to avoid a read-only var evaluated multiple times

XuHuaiyu · 2024-11-15T08:12:18Z

pkg/planner/core/preprocess.go

@@ -236,6 +242,9 @@ type preprocessor struct {

 	staleReadProcessor staleread.Processor

+	varsChanged  map[string]struct{}


varsWritable or varsMutable ?

XuHuaiyu · 2024-11-15T08:16:08Z

pkg/expression/builtin_other.go

+	if !isConst || arg0.DeferredExpr != nil {
+		return getVar
+	}
+	isReadonly := ctx.IsReadonlyUserVar(arg0.Value.GetString())


Suggested change

isReadonly := ctx.IsReadonlyUserVar(arg0.Value.GetString())

varName := arg.Value.GetString()

isReadonly := ctx.IsReadonlyUserVar(varName)

XuHuaiyu · 2024-11-15T08:16:23Z

pkg/expression/builtin_other.go

+		intest.Assert(false, "readonly user variable should not meet error when executing.")
+		return getVar
+	}
+	d, ok := ctx.GetEvalCtx().GetUserVarsReader().GetUserVarVal(arg0.Value.GetString())


Suggested change

d, ok := ctx.GetEvalCtx().GetUserVarsReader().GetUserVarVal(arg0.Value.GetString())

d, ok := ctx.GetEvalCtx().GetUserVarsReader().GetUserVarVal(varName)

XuHuaiyu · 2024-11-15T08:18:47Z

pkg/expression/builtin_other.go

+}
+
+// optimizeReadonlyVar tries to convert the readonly user variables to constants.
+func optimizeReadonlyVar(ctx BuildContext, getVar *ScalarFunction) Expression {


Suggested change

func optimizeReadonlyVar(ctx BuildContext, getVar *ScalarFunction) Expression {

func convertReadonlyVarToConst(ctx BuildContext, getVar *ScalarFunction) Expression {

ti-chi-bot · 2024-12-25T15:39:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qw4990, XuHuaiyu

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~~ [XuHuaiyu,qw4990]
~~pkg/expression/OWNERS~~ [XuHuaiyu]
~~pkg/planner/OWNERS~~ [qw4990]

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-12-25T15:39:06Z

[LGTM Timeline notifier]

Timeline:

2024-11-05 03:44:15.166733789 +0000 UTC m=+926168.005889336: ☑️ agreed by qw4990.
2024-12-25 15:39:05.000520812 +0000 UTC m=+1662535.089323372: ☑️ agreed by XuHuaiyu.

winoros · 2024-12-25T16:26:35Z

/retest

winoros · 2024-12-25T18:04:59Z

/cherrypick release-8.5

close pingcap#52742

ti-chi-bot · 2024-12-25T18:05:15Z

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

ti-chi-bot · 2024-12-25T18:05:56Z

@winoros: new pull request could not be created: failed to create pull request against pingcap/tidb#release-8.5 from head ti-chi-bot:cherry-pick-54462-to-release-8.5: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for ti-chi-bot:cherry-pick-54462-to-release-8.5."}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#create-a-pull-request","status":"422"}

In response to this:

/cherrypick release-8.5

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 ti-community-infra/tichi repository.

close pingcap#52742

) close #52742

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/planner SIG: Planner labels Jul 4, 2024

winoros force-pushed the eval-readonly-var branch from 1dfbb1c to 3830760 Compare July 4, 2024 21:49

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/needs-triage-completed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 4, 2024

winoros force-pushed the eval-readonly-var branch from 3830760 to cddf002 Compare July 8, 2024 16:46

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 Jul 8, 2024

winoros force-pushed the eval-readonly-var branch from cddf002 to 0561371 Compare July 8, 2024 17:02

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2024

winoros added 2 commits July 9, 2024 01:06

planner, expr: eval readonly user var during plan phase

6ba6125

Merge remote-tracking branch 'origin/master' into eval-readonly-var

c8dc5cb

winoros force-pushed the eval-readonly-var branch from 0561371 to c8dc5cb Compare July 8, 2024 17:09

ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2024

qw4990 reviewed Jul 30, 2024

View reviewed changes

pkg/planner/core/plan_cache.go Outdated Show resolved Hide resolved

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2024

winoros added 2 commits October 22, 2024 01:05

Merge branch 'master' into eval-readonly-var

fab2e53

refine the impl

ae03a7a

winoros changed the title ~~planner, expr: eval readonly user var during plan phase~~ planner, expr: eval readonly user var during plan phase | tidb-test=pr/2419 Oct 22, 2024

winoros commented Oct 22, 2024

View reviewed changes

pkg/expression/exprstatic/evalctx.go Outdated Show resolved Hide resolved

winoros force-pushed the eval-readonly-var branch from ad3cb2b to 753b5f2 Compare October 22, 2024 17:34

Merge remote-tracking branch 'origin/master' into eval-readonly-var

4911a52

winoros force-pushed the eval-readonly-var branch from 753b5f2 to 4911a52 Compare October 22, 2024 17:44

limit the scope

d92be4f

winoros mentioned this pull request Oct 22, 2024

Incorrect JSON_KEYS result on string constant #56788

Closed

Update pkg/expression/exprstatic/exprctx.go

2698c57

Co-authored-by: 王超 <[email protected]>

qw4990 reviewed Oct 29, 2024

View reviewed changes

pkg/planner/core/plan_cache.go Outdated Show resolved Hide resolved

winoros added 3 commits November 5, 2024 01:12

Merge remote-tracking branch 'origin/master' into eval-readonly-var

b5c3af9

try to simplify some codes

155a816

Merge branch 'eval-readonly-var' of https://github.com/winoros/tidb i…

7ed8fc9

…nto eval-readonly-var

qw4990 reviewed Nov 5, 2024

View reviewed changes

qw4990 approved these changes Nov 5, 2024

View reviewed changes

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

winoros commented Nov 5, 2024

View reviewed changes

XuHuaiyu reviewed Nov 15, 2024

View reviewed changes

winoros added 2 commits December 25, 2024 20:49

Merge branch 'master' into eval-readonly-var

e44a001

address comments

8aeca9d

XuHuaiyu approved these changes Dec 25, 2024

View reviewed changes

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

ti-chi-bot bot merged commit 9958228 into pingcap:master Dec 25, 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 Dec 25, 2024

winoros deleted the eval-readonly-var branch December 25, 2024 18:05

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Dec 25, 2024

planner, expr: eval readonly user var during plan phase (pingcap#54462)

2aea21b

close pingcap#52742

ti-chi-bot mentioned this pull request Dec 25, 2024

planner, expr: eval readonly user var during plan phase (#54462) | tidb-test=pr/2462 #58540

Merged

13 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tidb that referenced this pull request Dec 25, 2024

planner, expr: eval readonly user var during plan phase (pingcap#54462)

3e6e9c4

close pingcap#52742

ti-chi-bot bot pushed a commit that referenced this pull request Dec 26, 2024

planner, expr: eval readonly user var during plan phase (#54462) (#58540

8c97a5a

) close #52742

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner, expr: eval readonly user var during plan phase | tidb-test=pr/2419 #54462

planner, expr: eval readonly user var during plan phase | tidb-test=pr/2419 #54462

winoros commented Jul 4, 2024

codecov bot commented Jul 4, 2024 •

edited

Loading

ti-chi-bot bot commented Aug 30, 2024

winoros commented Nov 4, 2024

qw4990 Nov 5, 2024

winoros Nov 5, 2024

XuHuaiyu Nov 15, 2024

XuHuaiyu Nov 15, 2024

XuHuaiyu Nov 15, 2024

XuHuaiyu Nov 15, 2024

ti-chi-bot bot commented Dec 25, 2024

ti-chi-bot bot commented Dec 25, 2024

winoros commented Dec 25, 2024

winoros commented Dec 25, 2024

ti-chi-bot commented Dec 25, 2024

ti-chi-bot commented Dec 25, 2024

		@@ -236,6 +242,9 @@ type preprocessor struct {

		staleReadProcessor staleread.Processor

		varsChanged map[string]struct{}

	isReadonly := ctx.IsReadonlyUserVar(arg0.Value.GetString())
	varName := arg.Value.GetString()
	isReadonly := ctx.IsReadonlyUserVar(varName)

	d, ok := ctx.GetEvalCtx().GetUserVarsReader().GetUserVarVal(arg0.Value.GetString())
	d, ok := ctx.GetEvalCtx().GetUserVarsReader().GetUserVarVal(varName)

	func optimizeReadonlyVar(ctx BuildContext, getVar *ScalarFunction) Expression {
	func convertReadonlyVarToConst(ctx BuildContext, getVar *ScalarFunction) Expression {

planner, expr: eval readonly user var during plan phase | tidb-test=pr/2419 #54462

planner, expr: eval readonly user var during plan phase | tidb-test=pr/2419 #54462

Conversation

winoros commented Jul 4, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

codecov bot commented Jul 4, 2024 • edited Loading

Codecov Report

ti-chi-bot bot commented Aug 30, 2024

winoros commented Nov 4, 2024

qw4990 Nov 5, 2024

Choose a reason for hiding this comment

winoros Nov 5, 2024

Choose a reason for hiding this comment

XuHuaiyu Nov 15, 2024

Choose a reason for hiding this comment

XuHuaiyu Nov 15, 2024

Choose a reason for hiding this comment

XuHuaiyu Nov 15, 2024

Choose a reason for hiding this comment

XuHuaiyu Nov 15, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 25, 2024

ti-chi-bot bot commented Dec 25, 2024

[LGTM Timeline notifier]

winoros commented Dec 25, 2024

winoros commented Dec 25, 2024

ti-chi-bot commented Dec 25, 2024

ti-chi-bot commented Dec 25, 2024

codecov bot commented Jul 4, 2024 •

edited

Loading