Repository navigation
dumpling: support column filters for table exports - #70196
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note-none
|
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:
📝 WalkthroughWalkthroughColumn projection is replaced with JSON-driven column filtering. The CLI, configuration, validation, SQL generation, and dump metadata now use ChangesColumn filter export
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Config
participant Dumper
participant SQLProjection
participant TableMeta
CLI->>Config: provide --column-filter-file
Config->>Config: parse and compile JSON rules
Config->>Dumper: provide ColumnFilterConfig
Dumper->>SQLProjection: validate and apply filter
SQLProjection->>TableMeta: store selected and source metadata
TableMeta-->>Dumper: return dump metadata
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #70196 +/- ##
================================================
- Coverage 76.3227% 73.0638% -3.2589%
================================================
Files 2041 2078 +37
Lines 560306 604649 +44343
================================================
+ Hits 427641 441780 +14139
- Misses 131764 160875 +29111
- Partials 901 1994 +1093
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dumpling/export/column_selector.go`:
- Around line 121-131: The include-mode validation and unmatchedColumns flow
currently report normalized lowercase names instead of the column casing
supplied by the user. Preserve the original selector casing alongside the
lowercase matching keys in selector.columnSet, then use those original names
when constructing unmatchedColumns and the “included columns” error while
retaining case-insensitive matching.
In `@dumpling/export/dump.go`:
- Around line 265-271: Add the same column-selectors/no-schemas validation used
by config.ParseFromFlags to Dump() before validateColumnSelectors runs. When
conf.SQL is empty and conf.ColumnSelectors is non-nil, reject configurations
where NoSchemas is not enabled, while preserving the existing selector
validation and error propagation.
🪄 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 Plus
Run ID: b2e820e0-8073-4d13-9d68-a05110fe1610
📒 Files selected for processing (11)
dumpling/export/BUILD.bazeldumpling/export/column_selector.godumpling/export/config.godumpling/export/config_test.godumpling/export/dump.godumpling/export/dump_test.godumpling/export/ir.godumpling/export/ir_impl.godumpling/export/sql.godumpling/export/sql_test.godumpling/export/util_for_test.go
|
🔍 Starting code review for this PR... |
|
/retest |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dumpling/export/config_test.go (1)
103-107: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the case-sensitive assertion observable.
With only
c1, both a selector match and a case-sensitive table-matcher miss return the same result. Include an unselected column and assert it remains present whenDB1.T1does not matchdb1.t1.Proposed test adjustment
- selectedFields, err = selectors.applyToColumns("DB1", "T1", []string{"c1"}) + selectedFields, err = selectors.applyToColumns("DB1", "T1", []string{"c1", "c3"}) require.NoError(t, err) - require.Equal(t, []string{"c1"}, selectedFields) + require.Equal(t, []string{"c1", "c3"}, selectedFields)🤖 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 `@dumpling/export/config_test.go` around lines 103 - 107, Update the test around selectors.applyToColumns to include both a selected column and an unselected column in the input, then assert the unselected column remains when applying selectors to “DB1.T1” while the selector targets lowercase “db1.t1”. This makes the case-sensitive table-matcher miss observable while preserving the existing no-error assertions.Source: Coding guidelines
🤖 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 `@dumpling/export/config_test.go`:
- Around line 103-107: Update the test around selectors.applyToColumns to
include both a selected column and an unselected column in the input, then
assert the unselected column remains when applying selectors to “DB1.T1” while
the selector targets lowercase “db1.t1”. This makes the case-sensitive
table-matcher miss observable while preserving the existing no-error assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 350cf15e-e16a-4bac-8f60-379da3a002f5
📒 Files selected for processing (4)
dumpling/export/column_selector.godumpling/export/config_test.godumpling/export/dump_test.godumpling/export/sql_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
dumpling/export/dump.go (1)
265-270: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid the duplicate metadata sweep while consistency locks are held.
Validation runs before
TearDown, issuing serialSHOW COLUMNScalls for every base table;dumpTableMetafetches the same metadata again. Large lock/flush dumps therefore extend the source write-block window by a full extra schema sweep.
dumpling/export/dump.go#L265-L270: run validation only after the consistency lock is released, while still before tasks are submitted, or reuse cached metadata.dumpling/export/dump.go#L505-L516: cache or propagate writable-column metadata so validation and table-meta construction do not issue duplicate queries.🤖 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 `@dumpling/export/dump.go` around lines 265 - 270, The validation at dumpling/export/dump.go:265-270 currently performs a full metadata sweep while consistency locks are held; move validateColumnFilter out of the locked phase and run it after TearDown but before task submission. At dumpling/export/dump.go:505-516, avoid re-querying the same writable-column metadata by caching or propagating the metadata between validation and dumpTableMeta, preserving validation behavior while eliminating duplicate SHOW COLUMNS calls.
🤖 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 `@dumpling/export/config.go`:
- Around line 635-640: Enforce the column-filter mode contract at all affected
sites: in dumpling/export/config.go lines 635-640, reject a non-nil ColumnFilter
when SQL is non-empty in addition to the existing NoSchemas validation; in
dumpling/export/dump.go lines 265-270, add an independent early rejection for
ColumnFilter when either NoSchemas is false or SQL is non-empty before
consistency setup; in dumpling/export/config_test.go lines 135-141, update the
SQL-and-filter case to expect the incompatibility error.
In `@dumpling/export/dump.go`:
- Around line 1361-1364: Update dumpTableMeta so conf.ColumnFilter is supplied
to buildSelectFieldInfo only when table represents a base table. For views and
sequences, pass the unfiltered/empty filter value so matching include or exclude
rules are not applied after validation. Reuse the existing table-type
classification from TableInfo rather than changing validation behavior.
---
Duplicate comments:
In `@dumpling/export/dump.go`:
- Around line 265-270: The validation at dumpling/export/dump.go:265-270
currently performs a full metadata sweep while consistency locks are held; move
validateColumnFilter out of the locked phase and run it after TearDown but
before task submission. At dumpling/export/dump.go:505-516, avoid re-querying
the same writable-column metadata by caching or propagating the metadata between
validation and dumpTableMeta, preserving validation behavior while eliminating
duplicate SHOW COLUMNS calls.
🪄 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 Plus
Run ID: cba590e8-3af6-4ea8-add8-f56d50dc73ce
📒 Files selected for processing (7)
dumpling/export/column_filter.godumpling/export/config.godumpling/export/config_test.godumpling/export/dump.godumpling/export/dump_test.godumpling/export/sql.godumpling/export/sql_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
dumpling/export/config.go (1)
636-641: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce the column-filter mode contract at both boundaries.
SQL-mode dumps ignore
ColumnFilter, while directly constructedConfigvalues can still combine filtering with schema output. Reject filters withSQL != ""during parsing and again atDump()entry before consistency setup.
dumpling/export/config.go#L636-L641: rejectColumnFilterwhenconf.SQL != ""as well as when!conf.NoSchemas.dumpling/export/dump.go#L265-L269: independently rejectColumnFilterwhen schemas are enabled or SQL mode is selected.Proposed fix
- if conf.ColumnFilter != nil && !conf.NoSchemas { - return errors.New("--column-filter-file requires --no-schemas/-m") + if conf.ColumnFilter != nil && (!conf.NoSchemas || conf.SQL != "") { + return errors.New("--column-filter-file requires --no-schemas/-m and cannot be used with --sql") }+ if conf.ColumnFilter != nil && (!conf.NoSchemas || conf.SQL != "") { + return errors.New("column filter requires schema-less table dumping and cannot be used with SQL mode") + } baseConn := newBaseConn(metaConn, true, rebuildMetaConn)🤖 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 `@dumpling/export/config.go` around lines 636 - 641, Enforce the column-filter contract at both validation boundaries: in dumpling/export/config.go within the ParseColumnFilterFile handling, reject non-nil conf.ColumnFilter when either conf.SQL is non-empty or schemas are enabled; in dumpling/export/dump.go at Dump() entry, independently reject the same two incompatible states before consistency setup. Preserve the existing error behavior and ensure directly constructed Config values are validated.
🤖 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.
Duplicate comments:
In `@dumpling/export/config.go`:
- Around line 636-641: Enforce the column-filter contract at both validation
boundaries: in dumpling/export/config.go within the ParseColumnFilterFile
handling, reject non-nil conf.ColumnFilter when either conf.SQL is non-empty or
schemas are enabled; in dumpling/export/dump.go at Dump() entry, independently
reject the same two incompatible states before consistency setup. Preserve the
existing error behavior and ensure directly constructed Config values are
validated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0eb68276-0a94-4720-bfaf-3c1b7d986137
📒 Files selected for processing (6)
dumpling/export/config.godumpling/export/dump.godumpling/export/dump_test.godumpling/export/ir.godumpling/export/sql.godumpling/export/sql_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- dumpling/export/dump_test.go
- dumpling/export/sql_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dumpling/export/dump.go (1)
1394-1411: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset
errafter logging the harmless missing_tidb_rowidcheck.
SelectTiDBRowIDruns for TiDB sources even for views and sequences, and only theErrBadFieldpath returnsnil. For non-base tables, the laterGetColumnTypesblock is skipped, so any other error remains inerrand is returned fromdumpTableMeta.🐛 Proposed fix: reset `err` after logging the swallowed failure
if conf.ServerInfo.ServerType == version.ServerTypeTiDB { hasImplicitRowID, err = SelectTiDBRowID(tctx, conn, db, tbl) if err != nil { tctx.L().Info("check implicit rowID failed", zap.String("database", db), zap.String("table", tbl), log.ShortError(err)) + err = nil } }🤖 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 `@dumpling/export/dump.go` around lines 1394 - 1411, In dumpTableMeta, clear err after logging a SelectTiDBRowID failure that is intentionally ignored, so non-base tables do not return the stale error when GetColumnTypes is skipped. Preserve the existing logging and subsequent GetColumnTypes error handling.
🤖 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.
Outside diff comments:
In `@dumpling/export/dump.go`:
- Around line 1394-1411: In dumpTableMeta, clear err after logging a
SelectTiDBRowID failure that is intentionally ignored, so non-base tables do not
return the stale error when GetColumnTypes is skipped. Preserve the existing
logging and subsequent GetColumnTypes error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a08646c0-c8b1-4136-938b-3fc0e5b0ddcc
📒 Files selected for processing (3)
dumpling/export/dump.godumpling/export/dump_test.godumpling/export/sql.go
joechenrh
changed the title
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: 6
- Inline comments: 6
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
🟡 [Minor] (4)
- ParseColumnFilter now leaks the concrete rule slice instead of the ColumnFilter interface (pkg/util/table-filter/column_filter.go:34, pkg/util/table-filter/column_filter.go:29, dumpling/export/column_filter.go:119)
- Column-filter TOML is decoded permissively, so a misspelled
columnskey silently dumps all columns (dumpling/export/column_filter.go:31, dumpling/export/column_filter.go:79, dumpling/export/column_filter.go:94) - columnFilterConfig.compile exists only to serve test code (dumpling/export/column_filter.go:40, dumpling/export/config_test.go:69, dumpling/export/config_test.go:236)
- --column-filter help text omits the always-case-insensitive column matching contract documented for --column-filter-file (dumpling/export/config.go:388, dumpling/export/config.go:390)
ℹ️ [Info] (2)
- parseColumnFilterArgs' TOML re-wrapping trick has no comment explaining the singular "filter" key (dumpling/export/column_filter.go:76)
- ColumnFilter interface becomes an unused, confusingly-similar name next to the new ColumnFilterRules (pkg/util/table-filter/column_filter.go:23, pkg/util/table-filter/column_filter.go:34)
bfe3701 to
7d17b31
Compare
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Correctness and test-sufficiency review: I found one fail-open data-selection bug, one exported-API compatibility concern, and one metadata-query retry regression. The current tests pass but are not sufficient to cover these cases or the final SQL/CSV/Parquet output workflow.
| columnRules := c.matchColumnRules(database, table) | ||
|
|
||
| selectedIndexes := make([]int, 0, len(sourceColumns)) | ||
| if len(columnRules) == 0 { |
There was a problem hiding this comment.
[P1] Empty or omitted column rules fail open. compileForOption accepts a matched filter with columns omitted or empty, so matchColumnRules returns a zero-length list. This branch then treats it exactly like an unmatched table and exports every writable column. That contradicts #70255, which says that once a table matches, columns not matching a positive rule are not selected; for a projection intended to omit sensitive fields, an incomplete rule can therefore expose all columns. Please reject empty column lists during compilation, or return a separate matched flag so a matched empty rule reaches the existing no-writable-columns error. Add file and inline tests for both omitted columns and columns = [], and replace the current matcher-only test that expects all columns.
There was a problem hiding this comment.
Done in 327c14b . compileForOption now rejects omitted or empty columns for both inline and file rules, and the old matcher-only all-columns case was replaced with coverage for both invalid forms.
| ColumnTypes() []string | ||
| // ColumnNames returns the selected output column names. | ||
| ColumnNames() []string | ||
| // SourceColumnTypes returns all source writable column types for schema-based logic such as split keys. |
There was a problem hiding this comment.
[P2] Avoid breaking the exported metadata/parser contracts for this internal feature. Adding these methods to exported TableMeta makes every downstream implementation stop compiling. This PR also changes ParseColumnFilter from returning ColumnFilter to concrete ColumnFilterRules, which breaks consumers that retain the old function signature. Neither break is necessary for the backend-facing CLI feature: keep the existing TableMeta interface and use a private extended source-metadata interface/carrier internally, and preserve ParseColumnFilter(args) (ColumnFilter, error) while adding a separate composable rules API for Dumpling.
There was a problem hiding this comment.
Addressed the compatibility-sensitive parts in 327c14b . ParseColumnFilter keeps the old (ColumnFilter, error) signature, and Dumpling uses ParseColumnFilterRules when it needs ordered rules. The source-column access also moved off TableMeta into a private extension path. I restored the existing Dumpling writer contracts in cc21813 and cab8d16 instead of adding private adapter layers around ColumnInfos and GetRawBytes, since those interfaces already carried that metadata before this PR.
| err := db.QuerySQL(tctx, func(rows *sql.Rows) error { | ||
| err := db.queryRows(tctx, func(rows *sql.Rows) error { | ||
| var err error | ||
| colTypes, err = rows.ColumnTypes() |
There was a problem hiding this comment.
[P2] Propagate Rows.Close errors so metadata discovery can retry. This reads ColumnTypes() without advancing the LIMIT 1 result to EOF. queryRows checks rows.Err() before its deferred close, and the deferred rows.Close() return value is discarded; the previous implementation explicitly returned that close error. If the driver discovers a protocol/network failure while draining the unread row during close, this attempt is reported as successful and the connection is not reset/rebuilt. Please close explicitly before returning success, propagate the close error, then inspect rows.Err(). A sqlmock.Rows.CloseError regression test can pin the retry/error behavior.
There was a problem hiding this comment.
Done in 327c14b . GetColumnTypes now closes rows explicitly before returning success and propagates close or rows errors through the existing retry path. Added TestGetColumnTypesRetriesCloseError with sqlmock.Rows.CloseError to pin the behavior.
| ) | ||
| tctx, conf, pool := d.tctx, d.conf, d.dbHandle | ||
| tctx.L().Info("begin to run Dump", zap.Stringer("conf", conf)) | ||
| if len(conf.columnFilter.Filters) > 0 { |
There was a problem hiding this comment.
This validation appears redundant with Config.ParseFromFlags, where both --column-filter and --column-filter-file already call validateColumnFilterOptions. Please either remove this duplicate check, or add a short comment explaining why it must also be enforced at Dump() time—for example, because NewDumper retains the caller-owned *Config, whose exported SQL and NoSchemas fields can be mutated after parsing or construction. If the runtime guard is retained, it should use source-neutral wording rather than always passing flagColumnFilterFile, which can mislabel an inline --column-filter configuration.
There was a problem hiding this comment.
Kept the runtime guard and added a one-line comment in 327c14b . The guard is still needed because Dumpling can receive a Config built or mutated without ParseFromFlags. I kept it on validateColumnFilterOptions rather than adding a second validator just to customize the message, so the runtime path still reuses the same precondition logic as CLI parsing.
| type columnProjection struct { | ||
| sourceTypes []*sql.ColumnType | ||
| selectedTypes []*sql.ColumnType | ||
| selectField string |
There was a problem hiding this comment.
Could we rename this to selectList? “Select list” is the standard SQL grammar term for the fragment between SELECT and FROM, so it is more precise than the singular selectField and does not sound like a complete SQL statement. I would avoid selectListSQL for that reason. The resulting code also reads naturally: projection.selectList = "*" or projection.selectList = strings.Join(selectedFields, ","). This can remain an internal rename without changing the exported TableMeta method in this PR.
There was a problem hiding this comment.
I left this as selectField after another pass. It is not the pure SQL grammar term, but it matches the existing TableMeta.SelectedField() naming and the surrounding code treats it as the rendered comma-joined field string. Keeping the local name aligned with that API avoids churn in this PR.
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
maybe add some integration test under dumpling/tests
rest lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, OliverS929 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
approved
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
…quisite Include the prerequisite column filtering changes from pingcap#70196 with pingcap#70506, resolve committed cherry-pick conflict markers, and preserve release-branch writer interfaces.
4 participants
What problem does this PR solve?
Issue Number: close #70255
Problem Summary:
Cloud Export needs a backend-facing Dumpling primitive to support table-level column filtering so data files can omit selected columns while keeping existing table selection, split-key selection, and metadata behavior correct.
What changed and how does it work?
This PR adds Dumpling column filter inputs for data output projection. This is a backend-facing Dumpling interface while Cloud Export uses Dumpling, not a customer-facing Cloud API contract.
File form:
Inline CLI form:
Both forms compile into the same filter rules.
--column-filtercan be specified multiple times and is mutually exclusive with--column-filter-file.The matching rules follow TiCDC column selectors and Lightning table filters:
!exclusion rules;columns = ["*", "!col_to_skip"].When multiple
[[filters]]entries or inline--column-filterrules match the same table, later filters have higher priority, matching the existing filter rule behavior.The column filter is applied during a pre-dump column projection preparation step. Dumpling stores both source columns/types and selected columns/types per table:
The option is currently guarded to data-only table dumps:
--column-filterand--column-filter-filerequire--no-schemas/-mand cannot be used together with--sql.Check List
Tests
./tools/check/failpoint-go-test.sh dumpling/export -count=1go test ./pkg/util/table-filter -count=1git diff --checkSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.