stmtctx, *: change TypeCtx field to a private field
#47742
Conversation
YangKeao
added
the
release-note-none
ti-chi-bot
bot
added
the
size/L
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #47742 +/- ##
================================================
+ Coverage 71.8893% 72.7931% +0.9038%
================================================
Files 1398 1421 +23
Lines 405152 411584 +6432
================================================
+ Hits 291261 299605 +8344
+ Misses 94280 93118 -1162
+ Partials 19611 18861 -750
Flags with carried forward coverage won't be shown. Click here to find out more.
|
pkg/types/context/truncate.go
Outdated
| @@ -20,7 +20,7 @@ import ( | |||
| ) | |||
|
|
|||
| // HandleTruncate ignores or returns the error based on the Context state. | |||
| func (c *Context) HandleTruncate(err error) error { | |||
| func (c Context) HandleTruncate(err error) error { | |||
There was a problem hiding this comment.
Any reason to change the receiver type? I think it should keep the same with other methods
There was a problem hiding this comment.
Both are fine. This change makes it easier to write sc.TypeCtx().HandleTruncate()...
Maybe I should add a HandleTruncate method for sc 🤔 , it sounds better.
There was a problem hiding this comment.
PTAL, I changed it to c *Context, and add a new method HandleTruncate to the statement context.
There was a problem hiding this comment.
that HandleTruncate has been added to the statement context, is it possible to modify https://github.com/pingcap/tidb/blob/master/pkg/types/binary_literal.go#L104C60-L104C60 to the previous version to use statement context? This currently seems to cause panic in the benchmark test #47752
There was a problem hiding this comment.
@Ranxy How about construct a statement context for the benchmark? See https://github.com/pingcap/tidb/pull/47742/files#diff-8b7e9a313a56bfe64171ff65af9748cf09de5490b9605ab2f2ed37e53c622911R555
YangKeao
force-pushed
the
fix-47689
branch
2 times, most recently
from
3232f31 to
88db855
Compare
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>
ti-chi-bot
bot
added
size/XL
|
/check-issue-triage-complete |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lcwangchao, xhebox, 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:
Approvers can indicate their approval by writing |
|
/retest A data race (not related with this PR) is detected. #47776 |
What problem does this PR solve?
Issue Number: close #47698, close #47752
Problem Summary:
What is changed and how it works?
This PR changed the
TypeCtxinto a private field, and provide a functionTypeCtxto get the type context from statement context.Actually I quite doubt whether copying
TypeCtxfor each function call will cause significant performance degradation 🤔 . This change will also make it easier/more flexible to change fromTypeCtxto*TypeCtxif we found loading fullTypeCtxinto register (or copying in memory, if the size increases in the future) is slow (or theTypeCtxgets too big to copy) in the future.Check List
Tests
Simple code refractor. They are covered by existing unit tests and integration tests.