dxf, importinto: compare subtask task IDs as strings (#69884)#69912
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
@D3Hunter This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
|
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)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds canonical string conversion for DXF ChangesDXF task-key handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/dxf/framework/storage/converter.go (1)
94-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix typographical errors in the documentation comment.
💡 Proposed fix
-// due to history reason, the task_key column inside subtask tables are defined -// as varchar, but it actually stores the ID of the task which is integer. to -// make sure TiDB query can correct handle this, we need to convert it to string. +// due to historical reasons, the task_key column inside subtask tables is defined +// as varchar, but it actually stores the ID of the task which is an integer. To +// make sure TiDB queries can correctly handle this, we need to convert it to a string.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/dxf/framework/storage/converter.go` around lines 94 - 96, Correct the typographical errors in the documentation comment describing the subtask table task_key conversion, including grammar and wording, without changing the converter implementation or its meaning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/dxf/framework/storage/table_test.go`:
- Around line 303-354: Remove the unresolved Git conflict markers surrounding
the “prepare transition persists fields and exposes zero-row CAS” test in the
table test file, preserving the incoming test body and ensuring the file remains
valid Go syntax.
In `@pkg/dxf/importinto/BUILD.bazel`:
- Around line 113-117: Remove the unresolved Git conflict markers in the
shard_count assignment and retain the correct single value for shard_count
according to the intended branch change, leaving the BUILD target syntactically
valid.
---
Nitpick comments:
In `@pkg/dxf/framework/storage/converter.go`:
- Around line 94-96: Correct the typographical errors in the documentation
comment describing the subtask table task_key conversion, including grammar and
wording, without changing the converter implementation or its meaning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ead2db09-6074-4d19-8f81-3f8dda73001b
📒 Files selected for processing (18)
.agents/skills/tidb-test-guidelines/references/dxf-case-map.mdpkg/dxf/framework/integrationtests/framework_err_handling_test.gopkg/dxf/framework/integrationtests/modify_test.gopkg/dxf/framework/storage/converter.gopkg/dxf/framework/storage/history.gopkg/dxf/framework/storage/subtask_state.gopkg/dxf/framework/storage/table_test.gopkg/dxf/framework/storage/task_state.gopkg/dxf/framework/storage/task_table.gopkg/dxf/framework/testutil/table_util.gopkg/dxf/importinto/BUILD.bazelpkg/dxf/importinto/job.gopkg/dxf/importinto/job_testkit_test.gopkg/dxf/importinto/jobhistory/history.gopkg/dxf/importinto/jobhistory/history_test.gotests/realtikvtest/addindextest1/disttask_test.gotests/realtikvtest/addindextest3/temp_index_test.gotests/realtikvtest/importintotest4/import_summary_test.go
| <<<<<<< HEAD | ||
| shard_count = 27, | ||
| ======= | ||
| shard_count = 30, | ||
| >>>>>>> 66be7a615be (dxf, importinto: compare subtask task IDs as strings (#69884)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Resolve merge conflict markers.
The file contains unresolved Git merge conflict markers (<<<<<<< HEAD, =======, >>>>>>>) which will cause a build failure.
🐛 Proposed fix to resolve the conflict
-<<<<<<< HEAD
- shard_count = 27,
-=======
- shard_count = 30,
->>>>>>> 66be7a615be (dxf, importinto: compare subtask task IDs as strings (`#69884`))
+ shard_count = 30,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <<<<<<< HEAD | |
| shard_count = 27, | |
| ======= | |
| shard_count = 30, | |
| >>>>>>> 66be7a615be (dxf, importinto: compare subtask task IDs as strings (#69884)) | |
| shard_count = 30, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/dxf/importinto/BUILD.bazel` around lines 113 - 117, Remove the unresolved
Git conflict markers in the shard_count assignment and retain the correct single
value for shard_count according to the intended branch change, leaving the BUILD
target syntactically valid.
Signed-off-by: D3Hunter <jujj603@gmail.com>
|
Cherry-pick conflicts appear resolved; removing the |
ti-chi-bot
removed
the
do-not-merge/hold
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
/retest |
1 similar comment
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-nextgen-202603 #69912 +/- ##
===========================================================
Coverage ? 76.3548%
===========================================================
Files ? 1937
Lines ? 542707
Branches ? 0
===========================================================
Hits ? 414383
Misses ? 128324
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, GMHDBJD 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 |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
73b2477
into
pingcap:release-nextgen-202603
3 participants
This is an automated cherry-pick of #69884
What problem does this PR solve?
Issue Number: ref #61702, ref #69788
Problem Summary:
Subtask
task_keycolumns store decimal task IDs as strings, but several callers bind integer values. TiDB can coerce these comparisons toDOUBLE, so adjacent IDs above 2^53 may compare equal and equality predicates cannot use thetask_keyindex as intended.passing string also make sure the table index is used for querying
What changed and how does it work?
Check List
Tests
Test commands:
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
task_keyvalues across active and history tables.GetJobLastUpdateTimeand enhanced history/active filtering regression checks near 2^53.importintotest sharding configuration.