expression: add more optional properties for `EvalContext` #51725

lcwangchao · 2024-03-13T05:18:46Z

What problem does this PR solve?

Issue Number: ref #51477

What changed and how does it work?

Add more optional properties for EvalContext

OptPropSessionVars: to provide variable.SessionVars .
OptPropAdvisoryLock: to provide advisory lock operations.
OptPropDDLOwnerInfo: to provide DDL owner information.

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-03-13T05:19:03Z

Hi @lcwangchao. 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/test-infra repository.

codecov · 2024-03-13T05:51:02Z

Codecov Report

Merging #51725 (51237ea) into master (e25fb18) will decrease coverage by 15.8290%.
Report is 16 commits behind head on master.
The diff coverage is 39.2290%.

Additional details and impacted files

@@                Coverage Diff                @@
##             master     #51725         +/-   ##
=================================================
- Coverage   70.6646%   54.8357%   -15.8290%     
=================================================
  Files          1477       1592        +115     
  Lines        438720     614532     +175812     
=================================================
+ Hits         310020     336983      +26963     
- Misses       109181     254329     +145148     
- Partials      19519      23220       +3701

Flag	Coverage Δ
integration	`36.8467% <26.9841%> (?)`
unit	`70.4351% <35.1473%> (-0.0174%)`	⬇️

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

Components	Coverage Δ
dumpling	`53.9957% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`51.1029% <ø> (+5.3154%)`	⬆️

zimulala · 2024-03-18T04:29:07Z

pkg/expression/contextopt/sessionvars.go

+
+// GetSessionVars returns the session vars from the context
+func (SessionVarsPropReader) GetSessionVars(ctx context.EvalContext) (*variable.SessionVars, error) {
+	p, err := getPropProvider[*SessionVarsPropProvider](ctx, context.OptPropSessionVars)


The key itself must exist, what is the point of adding this judgment? The main thing is that an extra error judgment feels that the code is not very elegant.

If this is an impossible situation, is it better to just panic instead of return an error?

Specifically, when will a return error occur?

In the future, we may have multiple implementations for EvalContext and not all of them are required to contain all properties. The expression should fail when a property is not provided but required by an expression.

Usually, the error occurs when a coder constructs a EvalContext but missing to provide some required properties or using some unsupported expression in a specified scene. For example, only some simple expressions are allowed in a partition expression, but if someone uses function GetVar, it will fail.

Though in the above partition expression case, we should do some prior checks but we should also have a check in GetSessionVars to make the code strong. IMO, panic is not a good option because it will make tidb (at leat a session) crash.

Is it to be changed in the future, and panic is handled at present?

Is it to be changed in the future, and panic is handled at present?

I don't think it is necessary because EvalContext API is defined as "some property is optional" (You can see GetOptionalPropProvider returns bool to indicate it). It is natural for an expression to return an error because it does not need to care about some hard limitations about its callers.

If we want to do some asserts, it's better to do it somewhere else, for example, I've added check here:

tidb/pkg/expression/contextimpl/sessionctx.go

Line 51 in af76c2f

intest.Assert(impl.props.PropKeySet().IsFull())

to assert the ExprCtxExtendedImpl should have all optional properties...

lcwangchao · 2024-03-19T12:08:53Z

/retest

tiprow · 2024-03-19T12:09:15Z

@lcwangchao: 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/test-infra repository.

YangKeao

LGTM

windtalker

LGTM

ti-chi-bot · 2024-03-21T06:21:08Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [YangKeao,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-03-21T06:21:11Z

[LGTM Timeline notifier]

Timeline:

2024-03-20 08:35:10.085146458 +0000 UTC m=+1452137.107392846: ☑️ agreed by YangKeao.
2024-03-21 06:21:10.592022348 +0000 UTC m=+1530497.614268735: ☑️ agreed by windtalker.

ti-chi-bot · 2024-03-21T06:42:16Z

@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
idc-jenkins-ci-tidb/check_dev_2	`51237ea`	link	unknown	`/test check-dev2`

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.

lcwangchao · 2024-03-21T07:12:12Z

/retest

tiprow · 2024-03-21T07:12:33Z

@lcwangchao: 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/test-infra repository.

ti-chi-bot bot added do-not-merge/needs-triage-completed do-not-merge/needs-linked-issue 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 Mar 13, 2024

ti-chi-bot bot removed do-not-merge/needs-linked-issue do-not-merge/needs-triage-completed labels Mar 13, 2024

expression: add more optional properties for EvalContext

1be614f

lcwangchao force-pushed the more_opt_props branch from c50af36 to 1be614f Compare March 13, 2024 05:26

lcwangchao requested review from XuHuaiyu and YangKeao March 13, 2024 06:17

zimulala reviewed Mar 18, 2024

View reviewed changes

lcwangchao added 2 commits March 19, 2024 19:55

Merge branch 'master' into more_opt_props

713efa6

Add GetDivPrecisionIncrement

610efaf

YangKeao approved these changes Mar 20, 2024

View reviewed changes

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

Merge branch 'master' into more_opt_props

51237ea

windtalker approved these changes Mar 21, 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 Mar 21, 2024

ti-chi-bot bot merged commit 21e7939 into pingcap:master Mar 21, 2024
22 of 23 checks passed

lcwangchao deleted the more_opt_props branch March 21, 2024 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: add more optional properties for `EvalContext` #51725

expression: add more optional properties for `EvalContext` #51725

lcwangchao commented Mar 13, 2024 •

edited

Loading

tiprow bot commented Mar 13, 2024

codecov bot commented Mar 13, 2024 •

edited

Loading

zimulala Mar 18, 2024

Defined2014 Mar 18, 2024

lcwangchao Mar 18, 2024

zimulala Mar 18, 2024

lcwangchao Mar 18, 2024

lcwangchao commented Mar 19, 2024

tiprow bot commented Mar 19, 2024

YangKeao left a comment

windtalker left a comment

ti-chi-bot bot commented Mar 21, 2024

ti-chi-bot bot commented Mar 21, 2024

ti-chi-bot bot commented Mar 21, 2024

lcwangchao commented Mar 21, 2024

tiprow bot commented Mar 21, 2024

expression: add more optional properties for EvalContext #51725

expression: add more optional properties for EvalContext #51725

Conversation

lcwangchao commented Mar 13, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Mar 13, 2024

codecov bot commented Mar 13, 2024 • edited Loading

Codecov Report

zimulala Mar 18, 2024

Choose a reason for hiding this comment

Defined2014 Mar 18, 2024

Choose a reason for hiding this comment

lcwangchao Mar 18, 2024

Choose a reason for hiding this comment

zimulala Mar 18, 2024

Choose a reason for hiding this comment

lcwangchao Mar 18, 2024

Choose a reason for hiding this comment

lcwangchao commented Mar 19, 2024

tiprow bot commented Mar 19, 2024

YangKeao left a comment

Choose a reason for hiding this comment

windtalker left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Mar 21, 2024

ti-chi-bot bot commented Mar 21, 2024

[LGTM Timeline notifier]

ti-chi-bot bot commented Mar 21, 2024

lcwangchao commented Mar 21, 2024

tiprow bot commented Mar 21, 2024

expression: add more optional properties for `EvalContext` #51725

expression: add more optional properties for `EvalContext` #51725

lcwangchao commented Mar 13, 2024 •

edited

Loading

codecov bot commented Mar 13, 2024 •

edited

Loading