dumpling: support partition flag and generate dump sql based on partition condition#67618
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
|
@shiyuhang0 I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
component/dumpling
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as Config Parser
participant Validator as Validation Layer
participant Controller as Dump Controller
participant DumperSeq as Sequential Dumper
participant DumperConc as Concurrent Dumper
participant TiDB as TiDB
User->>CLI: provide flags (--partitions, --sql, ...)
CLI->>Validator: parse & normalize Config
Validator->>Validator: validate mutual exclusion, TiDB-only, version, partition existence
alt validation fails
Validator-->>User: error
else
Validator->>Controller: config OK
alt sequential mode
Controller->>DumperSeq: schedule per-partition dumps
loop per partition
DumperSeq->>TiDB: dumpWholeTableDirectly(partition)
TiDB-->>DumperSeq: rows
end
DumperSeq-->>User: complete
else concurrent mode (TiDB + TABLESAMPLE)
Controller->>DumperConc: prepare per-partition TABLESAMPLE queries
loop per partition
DumperConc->>TiDB: selectTiDBTableSample(partition)
TiDB-->>DumperConc: sampled handles
end
DumperConc->>DumperConc: dispatch concurrent tasks per partition
DumperConc-->>User: complete
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dumpling/export/dump.go`:
- Around line 503-505: The current pre-check using conf.ServerInfo.ServerVersion
and tableSampleVersion rejects all --partitions exports on <5.0.0; remove this
global early return and instead restrict the version requirement only where
partition-scoped chunking is used inside dumpTableData: keep the PARTITION(...)
direct export path reachable for older servers and, in the chunked-splitting
branches, gate behavior by checking
conf.ServerInfo.ServerVersion.Compare(*tableSampleVersion) and skip or alter the
partition-scoped chunking logic accordingly (i.e., bypass calling
buildConcatTask for servers < v5.0.0 and use the direct PARTITION export flow);
update the code paths that call buildConcatTask so they only do so when the
version check passes.
- Around line 422-424: The partition flag validation (d.checkPartitionsFlag
using tctx, metaConn, allTables) must run before any placement-policy tasks are
enqueued; move the call to d.checkPartitionsFlag so it executes prior to the
placement-policy task block that currently enqueues placement-policy tasks,
ensuring a config error aborts before any metadata tasks are queued.
🪄 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: 38d5be8e-9b15-401f-87f5-e0afe1cf8fe9
📒 Files selected for processing (4)
dumpling/export/config.godumpling/export/dump.godumpling/export/prepare_test.godumpling/export/sql_test.go
shiyuhang0
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67618 +/- ##
================================================
- Coverage 77.5796% 77.4224% -0.1573%
================================================
Files 1980 1966 -14
Lines 547722 556930 +9208
================================================
+ Hits 424921 431189 +6268
- Misses 121991 125706 +3715
+ Partials 810 35 -775
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
dumpling/export/dump.go (2)
504-509:⚠️ Potential issue | 🟠 MajorVersion check may be overly restrictive for simple partition exports.
The check at lines 507-509 rejects all
--partitionsusage for TiDB < 5.0.0. However, the simplePARTITION(...)path insequentialDumpTable(lines 771-779) doesn't require TABLESAMPLE and could work on older TiDB versions.If the TiDB 5.0.0 requirement is specifically for partition-scoped TABLESAMPLE splitting, consider gating only that path and allowing the direct partition export for older servers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dumpling/export/dump.go` around lines 504 - 509, The current top-level check rejects all --partitions usage for TiDB < v5.0.0; instead, modify the logic so that conf.ServerInfo.ServerType still enforces TiDB for any partitions usage but only require conf.ServerInfo.ServerVersion >= tableSampleVersion when code will use TABLESAMPLE-based splitting; locate the version compare that references conf.ServerInfo.ServerVersion and tableSampleVersion and move or duplicate it into the path in sequentialDumpTable (the TABLESAMPLE branch around the partition-scoped TABLESAMPLE logic) so the simple PARTITION(...) export path remains allowed on older TiDB versions.
426-428:⚠️ Potential issue | 🟡 MinorValidate
--partitionsbefore queueing any metadata tasks.This validation runs after placement-policy tasks (lines 401-424) can already be enqueued. A configuration error here could leave partial output behind. Consider moving the validation above the placement-policy block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dumpling/export/dump.go` around lines 426 - 428, The checkPartitionsFlag validation (call: d.checkPartitionsFlag(tctx, metaConn, allTables)) must run before any placement-policy tasks are enqueued; move this call above the placement-policy task creation/enqueue block so a bad --partitions config fails fast and prevents partial metadata tasks from being queued. Ensure you keep the same parameters (tctx, metaConn, allTables) and update control flow so that placement-policy task creation (the placement-policy enqueue logic) only executes after checkPartitionsFlag returns nil.
🧹 Nitpick comments (1)
dumpling/export/config.go (1)
86-86: Minor formatting inconsistency.This line uses spaces for indentation/alignment while the surrounding constant definitions use tabs. This may cause linter warnings.
- flagPartitions = "partitions" + flagPartitions = "partitions"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dumpling/export/config.go` at line 86, The constant declaration for flagPartitions uses spaces for indentation which is inconsistent with the other constants; update the declaration of flagPartitions so its indentation matches the surrounding constants (use a tab for alignment like the other entries) to satisfy formatting/linter rules and keep the constants block consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dumpling/export/dump.go`:
- Around line 504-509: The current top-level check rejects all --partitions
usage for TiDB < v5.0.0; instead, modify the logic so that
conf.ServerInfo.ServerType still enforces TiDB for any partitions usage but only
require conf.ServerInfo.ServerVersion >= tableSampleVersion when code will use
TABLESAMPLE-based splitting; locate the version compare that references
conf.ServerInfo.ServerVersion and tableSampleVersion and move or duplicate it
into the path in sequentialDumpTable (the TABLESAMPLE branch around the
partition-scoped TABLESAMPLE logic) so the simple PARTITION(...) export path
remains allowed on older TiDB versions.
- Around line 426-428: The checkPartitionsFlag validation (call:
d.checkPartitionsFlag(tctx, metaConn, allTables)) must run before any
placement-policy tasks are enqueued; move this call above the placement-policy
task creation/enqueue block so a bad --partitions config fails fast and prevents
partial metadata tasks from being queued. Ensure you keep the same parameters
(tctx, metaConn, allTables) and update control flow so that placement-policy
task creation (the placement-policy enqueue logic) only executes after
checkPartitionsFlag returns nil.
---
Nitpick comments:
In `@dumpling/export/config.go`:
- Line 86: The constant declaration for flagPartitions uses spaces for
indentation which is inconsistent with the other constants; update the
declaration of flagPartitions so its indentation matches the surrounding
constants (use a tab for alignment like the other entries) to satisfy
formatting/linter rules and keep the constants block consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a104b743-fb1e-48f2-9c59-2493fe70568c
📒 Files selected for processing (3)
dumpling/export/config.godumpling/export/dump.godumpling/export/sql_test.go
✅ Files skipped from review due to trivial changes (1)
- dumpling/export/sql_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dumpling/export/config.go (1)
811-813: Consider making the new conflict error more actionable for users.
Line 812is correct functionally, but adding a short remediation hint (similar to the--sql+--wheremessage) would improve CLI UX consistency.Suggested tweak
- return errors.New("can't specify both --sql and --partitions at the same time") + return errors.New("can't specify both --sql and --partitions at the same time. Please remove one of them")As per coding guidelines: "Keep error handling actionable and contextual; avoid silently swallowing errors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dumpling/export/config.go` around lines 811 - 813, The conflict error when both conf.SQL and conf.Partitions are set should be changed to include a short remediation hint for the user; update the error returned from the check that currently reads "can't specify both --sql and --partitions at the same time" (the block referencing conf.SQL and conf.Partitions) to a message that suggests the correct action (e.g., choose one flag or explain how to express filtering via --sql/WHERE vs using --partitions) so the CLI gives actionable guidance consistent with the --sql + --where message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dumpling/export/config.go`:
- Around line 811-813: The conflict error when both conf.SQL and conf.Partitions
are set should be changed to include a short remediation hint for the user;
update the error returned from the check that currently reads "can't specify
both --sql and --partitions at the same time" (the block referencing conf.SQL
and conf.Partitions) to a message that suggests the correct action (e.g., choose
one flag or explain how to express filtering via --sql/WHERE vs using
--partitions) so the CLI gives actionable guidance consistent with the --sql +
--where message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 274fb33e-6beb-4302-9433-d3c5182c2774
📒 Files selected for processing (1)
dumpling/export/config.go
shiyuhang0
changed the title
|
🔍 Starting code review for this PR... |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 10
- Inline comments: 10
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (6)
--partitionshelp text does not describe the actual contract enforced by the new code path (dumpling/export/config.go:384, dumpling/export/dump.go:523, dumpling/export/config.go:811)- Duplicate
--partitionsentries schedule duplicate partition dumps (dumpling/export/config.go:548, dumpling/export/dump.go:772, dumpling/export/dump.go:976) - Global partitions option is modeled as process-wide state but validated as an all-tables invariant (dumpling/export/config.go:190, dumpling/export/dump.go:523)
- Partitioned TABLESAMPLE dump path has no determinism regression test (dumpling/export/dump.go:791, dumpling/export/dump.go:964, dumpling/export/sql_test.go:525)
- Partition TABLESAMPLE path aborts dump instead of degrading on transient failures (dumpling/export/dump.go:792, dumpling/export/dump.go:964)
- Partition sampling repeats primary-key metadata lookup for every partition (dumpling/export/dump.go:979, dumpling/export/dump.go:1063, dumpling/export/sql.go:639)
🟡 [Minor] (4)
--partitionshelp text omits upgrade/compatibility constraints (dumpling/export/config.go:384, dumpling/export/dump.go:505, dumpling/export/dump.go:508)- Partition-related errors and logs drift from the canonical option vocabulary (dumpling/export/dump.go:505, dumpling/export/dump.go:805)
- Partition validation adds one metadata query per base table before dumping starts (dumpling/export/dump.go:426, dumpling/export/dump.go:515, dumpling/export/sql.go:1465)
- Partition chunk scheduling logic is duplicated across TABLESAMPLE and TABLE REGIONS paths (dumpling/export/dump.go:964, dumpling/export/dump.go:997)
| } | ||
| conf := d.conf | ||
| if conf.ServerInfo.ServerType != version.ServerTypeTiDB { | ||
| return errors.New("partition: dump with partitions flag is only available for TiDB") |
There was a problem hiding this comment.
🟡 [Minor] Partition-related errors and logs drift from the canonical option vocabulary
Impact
The new user-facing messages use mixed forms like partition: and partitions flag instead of the exact option token, which makes troubleshooting less direct. Operators matching runtime errors to CLI help must infer that these messages refer to --partitions.
Scope
dumpling/export/dump.go:505—(*Dumper).checkPartitionsFlagdumpling/export/dump.go:805—(*Dumper).concurrentDumpTable
Evidence
New messages include partition: dump with partitions flag is only available for TiDB, partition: dump with partitions flag requires TiDB version >= v5.0.0, and partition: dump with partitions flag does not support using rows. None of these strings names the option as --partitions, so the contract vocabulary in runtime errors does not match the CLI surface.
Change request
Standardize these error/log strings to use the canonical option name --partitions and keep singular/plural wording consistent. Include a concise reason phrase where needed so fallback-path failures are self-explanatory at the call site.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dumpling/export/config.go (1)
851-866: Consider whether lowercasing partition names is always safe.
normalizePartitionsunconditionally lowercases each entry. In TiDB, partition names are identifiers that are typically compared case-insensitively, so this is usually fine; however, the normalized values are later used both for validation against table metadata and for generatingPARTITION(...)clauses on the dump side. If downstream comparison/emission ever switches to case-sensitive matching (e.g. identifier quoting,lower_case_table_names=0-style semantics, or a future refactor that preserves casing from metadata), this normalization could mismatch. A brief comment on the function documenting the case-insensitive assumption, or deduping via a lowercased key while preserving the user's original casing inresult, would make the invariant explicit.Otherwise the helper is clean: trims, drops empties, dedups, and preserves input order.
♻️ Optional: preserve original casing while deduping case-insensitively
func normalizePartitions(partitions []string) []string { + // Partition names are treated case-insensitively for dedup/validation; + // keep the first-seen original casing so error messages reflect user input. seen := make(map[string]struct{}, len(partitions)) result := make([]string, 0, len(partitions)) for _, p := range partitions { - p = strings.ToLower(strings.TrimSpace(p)) - if p == "" { + trimmed := strings.TrimSpace(p) + if trimmed == "" { continue } - if _, ok := seen[p]; ok { + key := strings.ToLower(trimmed) + if _, ok := seen[key]; ok { continue } - seen[p] = struct{}{} - result = append(result, p) + seen[key] = struct{}{} + result = append(result, trimmed) } return result }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dumpling/export/config.go` around lines 851 - 866, normalizePartitions currently lowercases each partition name unconditionally which can break correctness if downstream code ever treats identifiers case-sensitively; modify normalizePartitions to dedupe using a lowercased key but preserve the user's original (trimmed) casing in the result: compute key := strings.ToLower(strings.TrimSpace(p)) for deduping in the seen map while appending the original trimmed p (not the lowercased one) to result, skip empties as before, and update the function comment to document the case-insensitive dedupe invariant (or, alternatively, explicitly state why unconditional lowercasing is acceptable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dumpling/export/config.go`:
- Around line 851-866: normalizePartitions currently lowercases each partition
name unconditionally which can break correctness if downstream code ever treats
identifiers case-sensitively; modify normalizePartitions to dedupe using a
lowercased key but preserve the user's original (trimmed) casing in the result:
compute key := strings.ToLower(strings.TrimSpace(p)) for deduping in the seen
map while appending the original trimmed p (not the lowercased one) to result,
skip empties as before, and update the function comment to document the
case-insensitive dedupe invariant (or, alternatively, explicitly state why
unconditional lowercasing is acceptable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: edb30813-4afc-47b9-9412-4074255f6482
📒 Files selected for processing (2)
dumpling/export/config.godumpling/export/dump.go
🚧 Files skipped from review as they are similar to previous changes (1)
- dumpling/export/dump.go
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
ti-chi-bot
Bot
added
size/XL
|
@shiyuhang0: The following test 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. |
|
[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
4 participants
What problem does this PR solve?
Issue Number: ref #67619
Also a part of this project: #67765
Problem Summary:
The export performance issue： when the user specifies the
whereflag to dump only one partation from a large table, dumpling performing full-table sampling (TABLESAMPLE REGIONS) to generate data chunks, which creates numerous redundant scan tasks for non-target partitions that consume TiKV resources without returning any data.What changed and how does it work?
Adds --partitions flag so users can explicitly specify the partitions they want to export. When partitions are provided, the PARTITION condition in TABLESAMPLE REGIONS() can be pushed down to tikv, which means region sampling is performed only on the selected partitions instead of the entire table. As a result, the generated chunk SQLs are scoped to the target partitions from the beginning, avoiding unnecessary full-table sampling and reducing the chance that execution falls back to inefficient partition-wide scans caused by where filters.
Key change:
--partitionsflag and reject it when used together with--sql.TABLESAMPLE REGIONS()queries. The partition condition inTABLESAMPLE REGIONS()can be pushed down to tikv. It only returns the regions where the partition is located.Performance
This feature has already been verified in TiDB Cloud.
Based on the test results for a customer with thousands of partitions (each ranging from 50GB to 200GB), the optimization yields the following performance breakthroughs.
before: dump a partition with
whereflagafter: dump a partition with
partationfalgKey Improvements （24 concurrency）
Manual Test
Use ossinsight datasize in TiDB Cloud cluster
dump
result
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests