lighting: replace lightning inner contexts with manged ones #56038

lcwangchao · 2024-09-12T07:52:15Z

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 wraps StaticExprContext to provide expression context.
litTableMutateContext provides table mutate context

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-09-12T07:52:33Z

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-sigs/prow repository.

lcwangchao · 2024-09-12T07:53:28Z

pkg/executor/importer/kv_encode.go

@@ -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()


This is not needed any more because we have removed SessionVars in the new session.

lcwangchao · 2024-09-12T07:59:13Z

pkg/lightning/backend/kv/context_test.go

+	compareWithLegacySession(tblCtx, sysVars)
+
+	// test for `RowEncodingConfig.IsRowLevelChecksumEnabled` which should be loaded from global variable.
+	require.False(t, variable.EnableRowLevelChecksum.Load())


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.

lcwangchao · 2024-09-12T08:00:52Z

pkg/lightning/backend/kv/session.go

@@ -312,6 +311,7 @@ func (*sessEvalContext) GetOptionalPropProvider(exprctx.OptionalEvalPropKey) (ex
 	return nil, false
 }

+// Deprecated: `session` will be removed soon.


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

lcwangchao · 2024-09-12T08:01:28Z

pkg/lightning/backend/kv/session.go

+	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.


To keep compatible, filter the valid system variables before constructing the session.

codecov · 2024-09-12T08:03:46Z

Codecov Report

Attention: Patch coverage is 76.38191% with 47 lines in your changes missing coverage. Please review.

Project coverage is 56.3953%. Comparing base (b3914c5) to head (b4b2445).
Report is 10 commits behind head on master.

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

Flag	Coverage Δ
integration	`38.7834% <67.8392%> (?)`
unit	`72.3576% <81.9672%> (+0.3001%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.9567% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`52.8854% <ø> (+7.0867%)`	⬆️

lcwangchao · 2024-09-12T08:04:34Z

pkg/lightning/backend/kv/context.go

+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
+}


PTAL @lance6716 @D3Hunter . some features not supported in lighthing/import into

lcwangchao · 2024-09-12T13:58:14Z

/retest

tiprow · 2024-09-12T13:58:40Z

@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-sigs/prow repository.

lcwangchao · 2024-09-13T03:23:20Z

/hold

Should waiting for #56059 merged first

pkg/lightning/backend/kv/context.go

lance6716 · 2024-09-13T06:14:34Z

pkg/lightning/backend/kv/context.go

+	return &ctx.reservedRowIDAlloc, true
+}
+
+// GetBinlogSupport implements the `table.MutateContext` interface.


this method will be soon removed by #55955

Yes, if #55955 merged first, I'll remove this implementation. Otherwise, #55955 should remove this ...

lance6716 · 2024-09-13T06:15:49Z

pkg/lightning/backend/kv/context.go

+}
+
+// UpdatePhysicalTableDelta implements the `table.StatisticsSupport` interface.
+func (ctx *litTableMutateContext) UpdatePhysicalTableDelta(


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.

I've simplified the implementation, please take a look again.

pkg/lightning/backend/kv/session.go

lcwangchao · 2024-09-13T09:49:13Z

/retest

tiprow · 2024-09-13T09:49:34Z

@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-sigs/prow repository.

lcwangchao · 2024-09-14T03:42:52Z

/unhold

YangKeao

LGTM

D3Hunter

rest lgtm

pkg/lightning/backend/kv/session_internal_test.go

pkg/lightning/backend/kv/context.go

ti-chi-bot · 2024-09-17T04:16:39Z

[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:

~~OWNERS~~ [D3Hunter,YangKeao]
~~lightning/OWNERS~~ [D3Hunter]
~~pkg/executor/importer/OWNERS~~ [D3Hunter]
~~pkg/lightning/OWNERS~~ [D3Hunter]

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-09-17T04:16:41Z

[LGTM Timeline notifier]

Timeline:

2024-09-14 07:26:43.525309586 +0000 UTC m=+686873.265733525: ☑️ agreed by YangKeao.
2024-09-17 04:16:40.0856727 +0000 UTC m=+934669.826096638: ☑️ agreed by D3Hunter.

tiprow · 2024-09-17T06:33:08Z

@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
fast_test_tiprow	`b4b2445`	link	true	`/test fast_test_tiprow`

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.

lance6716 · 2024-09-17T06:52:30Z

/retest

tiprow · 2024-09-17T06:52:52Z

@lance6716: 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-sigs/prow repository.

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 Sep 12, 2024

lcwangchao commented Sep 12, 2024

View reviewed changes

lcwangchao mentioned this pull request Sep 12, 2024

remove mock.Context usage in production code #53388

Open

lighting: replace lightning inner contexts with manged ones

da6ac29

lcwangchao force-pushed the replace_lit_ctx branch from 3b8c21a to da6ac29 Compare September 12, 2024 08:22

lcwangchao requested review from D3Hunter and lance6716 September 12, 2024 08:22

fix test

4a25337

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 13, 2024

lance6716 reviewed Sep 13, 2024

View reviewed changes

address comment

52ade43

lcwangchao force-pushed the replace_lit_ctx branch from 4ce7952 to 52ade43 Compare September 13, 2024 09:16

Merge branch 'master' into replace_lit_ctx

b4b2445

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2024

lcwangchao requested a review from YangKeao September 14, 2024 03:46

YangKeao approved these changes Sep 14, 2024

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm label Sep 14, 2024

D3Hunter reviewed Sep 14, 2024

View reviewed changes

pkg/lightning/backend/kv/session_internal_test.go Show resolved Hide resolved

pkg/lightning/backend/kv/context.go Show resolved Hide resolved

D3Hunter approved these changes Sep 17, 2024

View reviewed changes

ti-chi-bot bot added the lgtm label Sep 17, 2024

ti-chi-bot bot added approved and removed needs-1-more-lgtm labels Sep 17, 2024

ti-chi-bot bot merged commit 12e5b31 into pingcap:master Sep 17, 2024
23 of 24 checks passed

lcwangchao deleted the replace_lit_ctx branch September 18, 2024 02:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lighting: replace lightning inner contexts with manged ones #56038

lighting: replace lightning inner contexts with manged ones #56038

lcwangchao commented Sep 12, 2024

tiprow bot commented Sep 12, 2024

lcwangchao Sep 12, 2024

lcwangchao Sep 12, 2024

lcwangchao Sep 12, 2024

lcwangchao Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading

lcwangchao Sep 12, 2024

lcwangchao commented Sep 12, 2024

tiprow bot commented Sep 12, 2024

lcwangchao commented Sep 13, 2024

lance6716 Sep 13, 2024

lcwangchao Sep 13, 2024

lance6716 Sep 13, 2024

lcwangchao Sep 13, 2024

lcwangchao commented Sep 13, 2024

tiprow bot commented Sep 13, 2024

lcwangchao commented Sep 14, 2024

YangKeao left a comment

D3Hunter left a comment

ti-chi-bot bot commented Sep 17, 2024

ti-chi-bot bot commented Sep 17, 2024

tiprow bot commented Sep 17, 2024

lance6716 commented Sep 17, 2024

tiprow bot commented Sep 17, 2024

lighting: replace lightning inner contexts with manged ones #56038

lighting: replace lightning inner contexts with manged ones #56038

Conversation

lcwangchao commented Sep 12, 2024

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Sep 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcwangchao Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

lcwangchao commented Sep 12, 2024

tiprow bot commented Sep 12, 2024

lcwangchao commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcwangchao commented Sep 13, 2024

tiprow bot commented Sep 13, 2024

lcwangchao commented Sep 14, 2024

YangKeao left a comment

Choose a reason for hiding this comment

D3Hunter left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Sep 17, 2024

ti-chi-bot bot commented Sep 17, 2024

[LGTM Timeline notifier]

tiprow bot commented Sep 17, 2024

lance6716 commented Sep 17, 2024

tiprow bot commented Sep 17, 2024

lcwangchao Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 12, 2024 •

edited

Loading