expression: add `ctx` to `expression.Eval` #48609

lcwangchao · 2023-11-15T08:42:40Z

What problem does this PR solve?

Issue Number: close #48608

What is changed and how it works?

Add a new parameter ctx to expression.Eval
If we can get the ctx in the places calling Eval, just use it as the first paramter
We introduce a new method expression.EvalWithInnerCtx to replace the Eval when we cannot not find a valid ctx. expression.EvalWithInnerCtx is deprecated and we should remove it in the later PRs.

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

codecov · 2023-11-15T08:56:09Z

Codecov Report

Merging #48609 (1242d49) into master (b8db9a5) will increase coverage by 1.8808%.
Report is 7 commits behind head on master.
The diff coverage is 92.7710%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #48609        +/-   ##
================================================
+ Coverage   71.0843%   72.9651%   +1.8808%     
================================================
  Files          1365       1389        +24     
  Lines        404348     413600      +9252     
================================================
+ Hits         287428     301784     +14356     
+ Misses        96936      93022      -3914     
+ Partials      19984      18794      -1190

Flag	Coverage Δ
integration	`43.8653% <82.5301%> (?)`
unit	`71.4087% <90.9638%> (+0.3244%)`	⬆️

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

Components	Coverage Δ
dumpling	`54.1220% <ø> (+0.1345%)`	⬆️
parser	`∅ <ø> (∅)`
br	`49.0479% <0.0000%> (-4.0215%)`	⬇️

lcwangchao · 2023-11-15T10:32:37Z

/retest

lcwangchao · 2023-11-15T15:48:01Z

/test tiprow_fast_test

ti-chi-bot · 2023-11-15T15:48:05Z

@lcwangchao: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test check-dev
/test check-dev2
/test mysql-test
/test unit-test

The following commands are available to trigger optional jobs:

/test canary-notify-when-compatibility-sections-changed
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-br-integration-test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-ddl-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-mysql-client-test
/test pull-sqllogic-test
/test pull-tiflash-test

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/canary_ghpr_unit_test
pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test

In response to this:

/test tiprow_fast_test

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.

lcwangchao · 2023-11-16T01:04:33Z

/test tiprow_fast_test

ti-chi-bot · 2023-11-16T01:04:37Z

@lcwangchao: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test check-dev
/test check-dev2
/test mysql-test
/test unit-test

The following commands are available to trigger optional jobs:

/test canary-notify-when-compatibility-sections-changed
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-br-integration-test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-ddl-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-mysql-client-test
/test pull-sqllogic-test
/test pull-tiflash-test

Use /test all to run the following jobs that were automatically triggered:

pingcap/tidb/canary_ghpr_unit_test
pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test

In response to this:

/test tiprow_fast_test

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.

lcwangchao · 2023-11-17T03:35:03Z

pkg/expression/constant.go

+func (c *Constant) EvalWithInnerCtx(row chunk.Row) (types.Datum, error) {
+	var ctx sessionctx.Context
+	if c.DeferredExpr != nil {
+		if sf, sfOk := c.DeferredExpr.(*ScalarFunction); sfOk {
+			ctx = sf.GetCtx()
+		}
+	}
+	return c.Eval(ctx, row)
+}


EvalWithInnerCtx will use the the context in the ScalarFunction as the input parameter for Eval. If c.DefferedExpr is nil, the ctx will also be nil, don't worry because ctx will not be used in this case

EvalWithInnerCtx will be removed after we replaces all EvalWithInnerCtx with Eval in the later PRs

lcwangchao · 2023-11-17T03:35:32Z

pkg/expression/constant.go

+				intest.AssertNotNil(ctx)
 				if dt.Kind() != types.KindMysqlDecimal {
-					val, err := dt.ConvertTo(sf.GetCtx().GetSessionVars().StmtCtx.TypeCtx(), c.RetType)
+					val, err := dt.ConvertTo(ctx.GetSessionVars().StmtCtx.TypeCtx(), c.RetType)


We'll use the input ctx instead of the inner one after this PR

lcwangchao · 2023-11-17T03:37:09Z

pkg/expression/scalar_function.go

-				if sc := ctx.GetSessionVars().StmtCtx; sc != nil {
-					err = sc.HandleTruncate(err)
-				}
+			if sc := ctx.GetSessionVars().StmtCtx; sc != nil {


We'll use the ctx from parameter to replace the inner ctx

lance6716 · 2023-11-17T03:46:14Z

/approve

zimulala

LGTM

YangKeao

LGTM

ti-chi-bot · 2023-11-17T04:52:54Z

[LGTM Timeline notifier]

Timeline:

2023-11-17 03:53:35.446300331 +0000 UTC m=+4394013.033410461: ☑️ agreed by zimulala.
2023-11-17 04:52:53.101774218 +0000 UTC m=+4397570.688884360: ☑️ agreed by YangKeao.

ti-chi-bot · 2023-11-22T03:12:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716, tiancaiamao, XuHuaiyu, YangKeao, zimulala

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,YangKeao,lance6716,tiancaiamao,zimulala]
~~br/pkg/lightning/OWNERS~~ [lance6716]
~~pkg/ddl/OWNERS~~ [zimulala]
~~pkg/executor/importer/OWNERS~~ [lance6716]
~~pkg/expression/OWNERS~~ [XuHuaiyu]
~~pkg/table/OWNERS~~ [zimulala]

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

lcwangchao · 2023-11-22T03:47:53Z

/retest

lcwangchao · 2023-11-22T04:21:12Z

/retest

tiprow · 2023-11-22T04:28:23Z

@lcwangchao: 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
tiprow_fast_test	`1242d49`	link	true	`/test tiprow_fast_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/test-infra repository. I understand the commands that are listed here.

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 15, 2023

lcwangchao force-pushed the eval_ctx branch 3 times, most recently from 0dc4c5d to e4ae0f0 Compare November 15, 2023 08:55

lcwangchao force-pushed the eval_ctx branch from e4ae0f0 to f8489fd Compare November 15, 2023 09:06

expression: add ctx to expression.Eval

4618d6a

lcwangchao force-pushed the eval_ctx branch from f8489fd to 4618d6a Compare November 15, 2023 09:25

lcwangchao requested a review from YangKeao November 16, 2023 01:06

lcwangchao commented Nov 17, 2023

View reviewed changes

lcwangchao requested review from XuHuaiyu and zimulala November 17, 2023 03:48

zimulala approved these changes Nov 17, 2023

View reviewed changes

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

YangKeao approved these changes Nov 17, 2023

View reviewed changes

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

tiancaiamao approved these changes Nov 17, 2023

View reviewed changes

Merge branch 'master' into eval_ctx

1242d49

XuHuaiyu approved these changes Nov 22, 2023

View reviewed changes

ti-chi-bot bot added the approved label Nov 22, 2023

ti-chi-bot bot merged commit 9062834 into pingcap:master Nov 22, 2023

lcwangchao deleted the eval_ctx branch November 22, 2023 05:58

expression: add ctx to expression.Eval #48609

expression: add ctx to expression.Eval #48609

Uh oh!

Conversation

lcwangchao commented Nov 15, 2023

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

lcwangchao commented Nov 15, 2023

Uh oh!

lcwangchao commented Nov 15, 2023

Uh oh!

ti-chi-bot bot commented Nov 15, 2023

Uh oh!

lcwangchao commented Nov 16, 2023

Uh oh!

ti-chi-bot bot commented Nov 16, 2023

Uh oh!

lcwangchao Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

lcwangchao Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

lcwangchao Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

lcwangchao Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

lance6716 commented Nov 17, 2023

Uh oh!

zimulala left a comment

Choose a reason for hiding this comment

Uh oh!

YangKeao left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Nov 17, 2023

[LGTM Timeline notifier]

Uh oh!

ti-chi-bot bot commented Nov 22, 2023

Uh oh!

lcwangchao commented Nov 22, 2023

Uh oh!

lcwangchao commented Nov 22, 2023

Uh oh!

tiprow bot commented Nov 22, 2023

Uh oh!

Uh oh!

expression: add `ctx` to `expression.Eval` #48609

expression: add `ctx` to `expression.Eval` #48609

codecov bot commented Nov 15, 2023 •

edited

Loading