dxf, importinto: compare subtask task IDs as strings#69884
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note
📝 WalkthroughWalkthroughThis change standardizes DXF task ID parameters as decimal string keys for subtask storage queries and updates related tests. IMPORT INTO active/history lookups gain regression coverage for adjacent large task IDs near ChangesTask-key normalization
Estimated code review effort: 3 (Moderate) | ~20 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 |
ti-chi-bot
Bot
added
the
size/L
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/dxf/importinto/job.go (1)
404-407: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer
UNION ALLto avoid deduplication overhead.Since the outer query calculates the
MAX()of all returned timestamps, the implicit deduplication step ofUNIONis unnecessary. UsingUNION ALLskips the deduplication overhead and provides slightly better execution performance.♻️ Proposed refactor
- (select state_update_time from mysql.tidb_background_subtask where task_key = %? - union - select state_update_time from mysql.tidb_background_subtask_history where task_key = %? + (select state_update_time from mysql.tidb_background_subtask where task_key = %? + union all + select state_update_time from mysql.tidb_background_subtask_history where task_key = %?🤖 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/job.go` around lines 404 - 407, Update the SQL query in the task timestamp lookup to use UNION ALL instead of UNION, preserving the existing subqueries and outer MAX() behavior.
🤖 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.
Nitpick comments:
In `@pkg/dxf/importinto/job.go`:
- Around line 404-407: Update the SQL query in the task timestamp lookup to use
UNION ALL instead of UNION, preserving the existing subqueries and outer MAX()
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2bef91a2-639b-4d1b-b159-bdb1a89b8559
📒 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
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69884 +/- ##
================================================
- Coverage 76.3207% 73.9361% -2.3846%
================================================
Files 2041 2058 +17
Lines 559975 580556 +20581
================================================
+ Hits 427377 429241 +1864
- Misses 131697 150745 +19048
+ Partials 901 570 -331
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: joechenrh, wjhuang2016 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
[LGTM Timeline notifier]Timeline:
|
|
/retest |
|
/cherry-pick release-nextgen-202603 |
|
@D3Hunter: new pull request created to branch DetailsIn 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 ti-community-infra/tichi repository. |
4 participants
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