-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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.
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
lighting: replace lightning inner contexts with manged ones #56038
Conversation
Hi @lcwangchao. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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-sigs/prow repository. |
@@ -62,8 +62,6 @@ func NewTableKVEncoder( | |||
if err != nil { | |||
return nil, err | |||
} | |||
// we need a non-nil TxnCtx to avoid panic when evaluating set clause | |||
baseKVEncoder.SessionCtx.SetTxnCtxNotNil() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not needed any more because we have removed SessionVars
in the new session.
compareWithLegacySession(tblCtx, sysVars) | ||
|
||
// test for `RowEncodingConfig.IsRowLevelChecksumEnabled` which should be loaded from global variable. | ||
require.False(t, variable.EnableRowLevelChecksum.Load()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsRowLevelChecksumEnabled
is a little special. It's value is affected by the global var variable.EnableRowLevelChecksum
, see:
tidb/pkg/sessionctx/variable/session.go
Line 2285 in 00aac17
vars.EnableRowLevelChecksum = true |
variable.EnableRowLevelChecksum
may vary for different TiDB nodes if the variable is in reloading. So it can affect IMPORT INTO
statement which can be executed across nodes. But I don't think it's a big deal and to be compatible, we keep the same behavior here.
@@ -312,6 +311,7 @@ func (*sessEvalContext) GetOptionalPropProvider(exprctx.OptionalEvalPropKey) (ex | |||
return nil, false | |||
} | |||
|
|||
// Deprecated: `session` will be removed soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have decided to remove the old session
from the next PR. In this PR, it is used in some tests to check the old and new session's behavior are the same
if options.SysVars != nil { | ||
// This sessVars is only used to do validations. | ||
sessVars := variable.NewSessionVars(nil) | ||
// To keep compatible with the old versions, we should to skip errors caused by illegal system variables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep compatible, filter the valid system variables before constructing the session.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #56038 +/- ##
=================================================
- Coverage 72.9443% 56.3953% -16.5491%
=================================================
Files 1609 1762 +153
Lines 447171 640408 +193237
=================================================
+ Hits 326186 361160 +34974
- Misses 100907 254496 +153589
- Partials 20078 24752 +4674
Flags with carried forward coverage won't be shown. Click here to find out more.
|
func (*litTableMutateContext) GetCachedTableSupport() (tbctx.CachedTableSupport, bool) { | ||
// lightning import does not support cached table. | ||
return nil, false | ||
} | ||
|
||
func (*litTableMutateContext) GetTemporaryTableSupport() (tbctx.TemporaryTableSupport, bool) { | ||
// lightning import does not support temporary table. | ||
return nil, false | ||
} | ||
|
||
func (*litTableMutateContext) GetExchangePartitionDMLSupport() (tbctx.ExchangePartitionDMLSupport, bool) { | ||
// lightning import is not in a DML query, we do not need to support it. | ||
return nil, false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL @lance6716 @D3Hunter . some features not supported in lighthing/import into
3b8c21a
to
da6ac29
Compare
/retest |
@lcwangchao: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
/hold Should waiting for #56059 merged first |
return &ctx.reservedRowIDAlloc, true | ||
} | ||
|
||
// GetBinlogSupport implements the `table.MutateContext` interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method will be soon removed by #55955
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
// UpdatePhysicalTableDelta implements the `table.StatisticsSupport` interface. | ||
func (ctx *litTableMutateContext) UpdatePhysicalTableDelta( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think lightning don't need implement whole logic. It only need column size by GetColumnSize. Or if we can maintain column size elsewhere this function can be a no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've simplified the implementation, please take a look again.
4ce7952
to
52ade43
Compare
/retest |
@lcwangchao: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
/unhold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, 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:
Approvers can indicate their approval by writing |
@lcwangchao: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
/retest |
@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
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. |
What problem does this PR solve?
Issue Number: ref #53388
What changed and how does it work?
This RP replaces some internal contexts for lighting/import into to make sure to contexts are simple and easier to maintain, including:
litExprContext
which wrapsStaticExprContext
to provide expression context.litTableMutateContext
provides table mutate contextCheck List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.