Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

expression, *: Eval param get type with context #53656

Merged
merged 2 commits into from
Jun 3, 2024
Merged

expression, *: Eval param get type with context #53656

merged 2 commits into from
Jun 3, 2024

Conversation

YangKeao
Copy link
Member

What problem does this PR solve?

Issue Number: close #53653

Problem Summary:

Ref #53533 and #53534. This PR splits the first commit of #53534 to make it easier to review.

The goal is to remove the sessionctx.Session inside the Constant. Instead, we should pass the session context through argument, especially for function String() and GetType(). This PR modifies the GetType() part. To make sure this patch can work standalone, the internal session in Constant is not removed yet. It'll be removed in #53534 (which is ready, but is too long to review).

What changed and how does it work?

This PR adds an argument EvalType to the method GetType in Expression, so that in the future, the

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.

    This refactor should be covered by existing tests. It shouldn't change any logic.

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 29, 2024
@YangKeao YangKeao force-pushed the eval-param-get-type-with-context branch 3 times, most recently from d3c5ab5 to e60bc17 Compare May 29, 2024 09:46
@codecov Codecov
Copy link

codecov bot commented May 29, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 94.90000% with 51 lines in your changes are missing coverage. Please review.

Project coverage is 74.5390%. Comparing base (fc3132c) to head (9057f3f).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@               Coverage Diff                @@
##             master     #53656        +/-   ##
================================================
+ Coverage   72.5198%   74.5390%   +2.0191%     
================================================
  Files          1506       1506                
  Lines        431071     431252       +181     
================================================
+ Hits         312612     321451      +8839     
+ Misses        99070      89881      -9189     
- Partials      19389      19920       +531
Flag Coverage Δ
integration 49.2128% <83.3000%> (?)
unit 71.4226% <85.8000%> (-0.0829%) ⬇️

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

Components Coverage Δ
dumpling 52.9656% <ø> (-1.0302%) ⬇️
parser ∅ <ø> (∅)
br 50.4911% <ø> (+8.6882%) ⬆️

@YangKeao YangKeao force-pushed the eval-param-get-type-with-context branch 2 times, most recently from a4c6738 to f576ad6 Compare May 29, 2024 10:50
qw4990
qw4990 reviewed May 30, 2024
View reviewed changes
pkg/executor/aggfuncs/func_group_concat.go
@@ -429,6 +429,8 @@ type partialResult4GroupConcatOrder struct {

type groupConcatOrder struct {
baseGroupConcat4String
ctors []collate.Collator
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add these 2 new fields?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because in func (e *groupConcatOrder) AllocPartialResult(), it'll use these fields to allocate partial result. Previously, it was generated in AllocPartialResult. However, add an argument EvalContext to AllocPartialResult will lead to tons of changes, so I choose to calculate the ctors and desc when this struct is constructed.

@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label May 30, 2024
@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 30, 2024
@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented May 30, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-05-30 06:46:48.371110777 +0000 UTC m=+2931762.128246349: ☑️ agreed by qw4990.
  • 2024-05-30 07:24:13.853344394 +0000 UTC m=+2934007.610479961: ☑️ agreed by lcwangchao.

@YangKeao YangKeao requested a review from zimulala May 31, 2024 02:23
zimulala
zimulala approved these changes May 31, 2024
View reviewed changes
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot
Copy link

ti-chi-bot bot commented Jun 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716, lcwangchao, qw4990, XuHuaiyu, 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:

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 ti-chi-bot bot added the approved label Jun 3, 2024
@YangKeao
eval parameter with context
2896454 
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
@YangKeao YangKeao force-pushed the eval-param-get-type-with-context branch from f576ad6 to c216499 Compare June 3, 2024 06:59
@YangKeao
add argument 'ctx' in the GetType method
9057f3f 
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
@YangKeao YangKeao force-pushed the eval-param-get-type-with-context branch from c216499 to 9057f3f Compare June 3, 2024 07:05
@YangKeao
Copy link
Member Author

YangKeao commented Jun 3, 2024

/retest

@YangKeao
Copy link
Member Author

YangKeao commented Jun 3, 2024

/retest

@ti-chi-bot ti-chi-bot bot merged commit 5e6df4e into pingcap:master Jun 3, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XuHuaiyu XuHuaiyu XuHuaiyu approved these changes

@qw4990 qw4990 qw4990 approved these changes

@zimulala zimulala zimulala approved these changes

@lance6716 lance6716 lance6716 approved these changes

@lcwangchao lcwangchao lcwangchao approved these changes

Assignees
No one assigned
Labels
approved lgtm release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add EvalContext argument for Expression.GetType
6 participants
@YangKeao @lcwangchao @lance6716 @zimulala @qw4990 @XuHuaiyu