stmtctx: remove OverflowAsWarning flag #49122

YangKeao · 2023-12-04T06:44:04Z

What problem does this PR solve?

Issue Number: close #49143, close #49137

Problem Summary:

The difference between OverflowAsWarning and TruncatedAsWarning doesn't actually exist. The existing difference are all incompatible with MySQL.

I suggest to read #49141 (comment). It gives a clearer explanation on why this PR is right.

What changed and how does it work?

This PR removes OverflowAsWarning and handle them through HandleError. Actually HandleError and HandleTruncate are the same (and HandleTruncate will be removed in this or next week). As we provided HandleErrorWithAlias, it's more appropriate for this PR.

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.

Fix the issue that in non-strict SQL_MODE, overflow error is not regarded as warning in UPDATE, DELETE and INSERT statement.

ti-chi-bot · 2023-12-04T06:44:08Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

YangKeao · 2023-12-04T06:44:12Z

/retest

tiprow · 2023-12-04T06:44:23Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-12-04T07:08:03Z

Codecov Report

Merging #49122 (8da014e) into master (d076bc1) will increase coverage by 0.5732%.
Report is 8 commits behind head on master.
The diff coverage is 57.8947%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #49122        +/-   ##
================================================
+ Coverage   70.9870%   71.5603%   +0.5732%     
================================================
  Files          1368       1417        +49     
  Lines        399790     417923     +18133     
================================================
+ Hits         283799     299067     +15268     
- Misses        96179     100033      +3854     
+ Partials      19812      18823       -989

Flag	Coverage Δ
integration	`43.9565% <57.8947%> (?)`
unit	`70.9879% <ø> (+0.0009%)`	⬆️

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

Components	Coverage Δ
dumpling	`53.9663% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`47.7150% <ø> (-5.1699%)`	⬇️

YangKeao · 2023-12-06T03:59:28Z

br/pkg/lightning/backend/kv/session.go

@@ -315,7 +313,6 @@ func NewSession(options *encode.SessionOptions, logger log.Logger) *Session {
 }
 }
 vars.StmtCtx.SetTimeZone(vars.Location())
- vars.StmtCtx.SetTypeFlags(types.StrictFlags)


This line is introduced in #47842. I think it's a bug, right? @lcwangchao Please confirm.

Seems to be a mistake, but introduced in previous PR: #47543 . In #47543 it overrides the type context settings before it...

YangKeao · 2023-12-06T04:09:24Z

/retest

wuhuizuo · 2023-12-06T04:16:03Z

/test unit-test

tiprow · 2023-12-06T04:16:17Z

@wuhuizuo: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test unit-test

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 · 2023-12-06T05:40:13Z

/retest

lance6716

/approve

for lightning part I'll check the integration test results

lance6716 · 2023-12-06T06:54:09Z

/test pull-lightning-integration-test

tiprow · 2023-12-06T06:54:23Z

@lance6716: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test pull-lightning-integration-test

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.

zimulala · 2023-12-06T06:59:38Z

pkg/ddl/backfilling_scheduler.go

@@ -196,7 +195,6 @@ func restoreSessCtx(sessCtx sessionctx.Context) func(sessCtx sessionctx.Context)
 timezone = &tz
 }
 badNullAsWarn := sv.StmtCtx.BadNullAsWarning
- overflowAsWarn := sv.StmtCtx.OverflowAsWarning


Here we remove OverflowAsWarning, do we need to add WithTruncateAsWarning?

WithTruncateAsWarning is part of typeFlags. Here we restored the typeFlags, so it's fine to just remove OverflowAsWarning.

lance6716 · 2023-12-06T07:40:47Z

/retest

SeaRise · 2023-12-11T10:15:54Z

/cc @SeaRise

Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>

SeaRise

lgtm

ti-chi-bot · 2023-12-14T07:49:45Z

[LGTM Timeline notifier]

Timeline:

2023-12-14 06:35:44.862718931 +0000 UTC m=+510835.899946051: ☑️ agreed by tiancaiamao.
2023-12-14 07:49:44.617806322 +0000 UTC m=+515275.655033243: ☑️ agreed by SeaRise.

tiprow · 2023-12-14T07:57:56Z

@YangKeao: 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
tiprow_fast_test	`8da014e`	link	true	`/test tiprow_fast_test`

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.

windtalker

LGTM

ti-chi-bot · 2023-12-14T15:50:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lance6716, SeaRise, tiancaiamao, windtalker, 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:

~~OWNERS~~ [lance6716,tiancaiamao,windtalker,zimulala]
~~br/pkg/lightning/OWNERS~~ [lance6716]
~~pkg/ddl/OWNERS~~ [zimulala]
~~pkg/expression/OWNERS~~ [windtalker]
~~pkg/table/OWNERS~~ [zimulala]

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 bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Dec 4, 2023

ti-chi-bot bot added the release-note-none Denotes a PR that doesn't merit a release note. label Dec 4, 2023

ti-chi-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 4, 2023

YangKeao force-pushed the remove-overflow-as-warning branch 2 times, most recently from f734a78 to 95fd483 Compare December 4, 2023 06:50

YangKeao force-pushed the remove-overflow-as-warning branch from 95fd483 to b8ee154 Compare December 4, 2023 07:38

ti-chi-bot bot removed do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked labels Dec 4, 2023

YangKeao marked this pull request as ready for review December 4, 2023 13:16

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2023

YangKeao force-pushed the remove-overflow-as-warning branch 6 times, most recently from 51a3bbb to c5856a5 Compare December 5, 2023 08:14

YangKeao removed the release-note-none Denotes a PR that doesn't merit a release note. label Dec 6, 2023

ti-chi-bot bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 6, 2023

YangKeao commented Dec 6, 2023

View reviewed changes

YangKeao force-pushed the remove-overflow-as-warning branch from c5856a5 to 581271b Compare December 6, 2023 04:04

lance6716 reviewed Dec 6, 2023

View reviewed changes

zimulala reviewed Dec 6, 2023

View reviewed changes

YangKeao requested review from zimulala and windtalker December 6, 2023 09:16

ti-chi-bot bot requested a review from SeaRise December 11, 2023 10:15

YangKeao mentioned this pull request Dec 13, 2023

executor: set OverflowAsWarning for insert statement in non-strict sql mode #49383

Merged

13 tasks

tiancaiamao approved these changes Dec 14, 2023

View reviewed changes

ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Dec 14, 2023

remove OverflowAsWarning flag

8da014e

Signed-off-by: Yang Keao <yangkeao@chunibyo.icu>

YangKeao force-pushed the remove-overflow-as-warning branch from 581271b to 8da014e Compare December 14, 2023 07:08

SeaRise approved these changes Dec 14, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Dec 14, 2023

windtalker approved these changes Dec 14, 2023

View reviewed changes

lcwangchao mentioned this pull request Dec 14, 2023

refactor types package to decouple it from stmtctx #47355

Closed

15 tasks

zimulala approved these changes Dec 14, 2023

View reviewed changes

ti-chi-bot bot added the approved label Dec 14, 2023

ti-chi-bot bot merged commit 531a1ce into pingcap:master Dec 14, 2023
23 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stmtctx: remove OverflowAsWarning flag #49122

stmtctx: remove OverflowAsWarning flag #49122

YangKeao commented Dec 4, 2023 •

edited

Loading

ti-chi-bot bot commented Dec 4, 2023

YangKeao commented Dec 4, 2023

tiprow bot commented Dec 4, 2023

codecov bot commented Dec 4, 2023 •

edited

Loading

YangKeao Dec 6, 2023

lcwangchao Dec 6, 2023

YangKeao commented Dec 6, 2023

wuhuizuo commented Dec 6, 2023

tiprow bot commented Dec 6, 2023

YangKeao commented Dec 6, 2023

lance6716 left a comment

lance6716 commented Dec 6, 2023

tiprow bot commented Dec 6, 2023

zimulala Dec 6, 2023

YangKeao Dec 6, 2023

lance6716 commented Dec 6, 2023

SeaRise commented Dec 11, 2023

SeaRise left a comment

ti-chi-bot bot commented Dec 14, 2023

tiprow bot commented Dec 14, 2023

windtalker left a comment

ti-chi-bot bot commented Dec 14, 2023

stmtctx: remove OverflowAsWarning flag #49122

stmtctx: remove OverflowAsWarning flag #49122

Conversation

YangKeao commented Dec 4, 2023 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

ti-chi-bot bot commented Dec 4, 2023

YangKeao commented Dec 4, 2023

tiprow bot commented Dec 4, 2023

codecov bot commented Dec 4, 2023 • edited Loading

Codecov Report

YangKeao Dec 6, 2023

Choose a reason for hiding this comment

lcwangchao Dec 6, 2023

Choose a reason for hiding this comment

YangKeao commented Dec 6, 2023

wuhuizuo commented Dec 6, 2023

tiprow bot commented Dec 6, 2023

YangKeao commented Dec 6, 2023

lance6716 left a comment

Choose a reason for hiding this comment

lance6716 commented Dec 6, 2023

tiprow bot commented Dec 6, 2023

zimulala Dec 6, 2023

Choose a reason for hiding this comment

YangKeao Dec 6, 2023

Choose a reason for hiding this comment

lance6716 commented Dec 6, 2023

SeaRise commented Dec 11, 2023

SeaRise left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 14, 2023

[LGTM Timeline notifier]

tiprow bot commented Dec 14, 2023

windtalker left a comment

Choose a reason for hiding this comment

ti-chi-bot bot commented Dec 14, 2023

YangKeao commented Dec 4, 2023 •

edited

Loading

codecov bot commented Dec 4, 2023 •

edited

Loading