executor: return proper error for LOAD DATA with AUTO_RANDOM column - #65597
ti-chi-bot[bot] merged 3 commits into
Conversation
|
Hi @Milad93R. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
Welcome @Milad93R! |
|
Hi @Milad93R. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions 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. |
|
/ok-to-test |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #65597 +/- ##
================================================
- Coverage 77.8487% 77.1389% -0.7098%
================================================
Files 1978 1942 -36
Lines 542181 568618 +26437
================================================
+ Hits 422081 438626 +16545
- Misses 118441 129919 +11478
+ Partials 1659 73 -1586
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
|
|
/retest |
|
CI failed: |
There was a problem hiding this comment.
I found this issue is caused by this TODO which I left some years ago 😢
@joechenrh Do you think we should open another PR, to let caller skip nil row?
There was a problem hiding this comment.
I'm wondering when we will reach L591 after this PR. Maybe we should return newRow, nil for restrictive data interpretation error 🤔?
There was a problem hiding this comment.
newRow may still have mismatched length. Maybe a row of NULL?
When using LOAD DATA LOCAL INFILE on a table with AUTO_RANDOM column while allow_auto_random_explicit_insert is disabled, return error 8216 (ErrInvalidAutoRandom) instead of a runtime panic. Previously, when the AUTO_RANDOM validation failed, the error was treated as a warning in non-restrictive mode, and a nil row was returned. This nil row later caused a panic when looking up the index with "runtime error: index out of range [0] with length 0". This change ensures that ErrInvalidAutoRandom is always returned as a real error, providing the user with a meaningful error message instead of a cryptic runtime panic. Issue Number: close pingcap#65585
27ec5d9 to
8ac26dd
Compare
|
C:/Program Files/Git/retest-required |
|
C:/Program Files/Git/test unit-test |
|
/retest |
|
/cc @kennytm @joechenrh |
| // when looking up index. See https://github.com/pingcap/tidb/issues/65585 | ||
| if dbterror.ErrInvalidAutoRandom.Equal(err) { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🤔 feels strange to only special case for the ErrInvalidAutoRandom
i think we should always return nil, err regardless of error type in strict mode, and discard the row in IGNORE mode
@joechenrh why is the current behavior like this 😕
There was a problem hiding this comment.
why is the current behavior like this
😵💫 I don't know about this part
Signed-off-by: Ruihao Chen <joechenrh@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughExplicitly handle ErrInvalidAutoRandom during LOAD DATA parsing to return the proper error instead of falling through to warning/fallback logic. Adds a test asserting the error and updates the test BUILD file. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Ruihao Chen <joechenrh@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joechenrh, lance6716 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@Milad93R: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
What problem does this PR solve?
Issue Number: close #65585
Problem Summary: When using
LOAD DATA LOCAL INFILEon a table withAUTO_RANDOMcolumn whileallow_auto_random_explicit_insertis disabled, TiDB returns a runtime panic (index out of range [0] with length 0) instead of the expected error 8216.What changed and how does it work?
Added a check in
parserData2TableData()to detectErrInvalidAutoRandomand return it as a real error instead of treating it as a warning. Previously, the error was converted to a warning in non-restrictive mode, and a nil row was returned, which later caused a panic when looking up the index.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Tests
Chores