cli: add TOML output format to changefeed query#4986
cli: add TOML output format to changefeed query#4986JesseZhuang wants to merge 2 commits intopingcap:masterfrom
Conversation
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds TOML output support to ChangesChangefeed Query TOML Output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Suggested labels
Suggested reviewers
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds TOML output support to the cdc cli changefeed query command, including a new --output flag and specialized handling for kebab-case keys and human-readable timestamps. Reviewers suggested addressing an inconsistency where simplified TOML output fails to use kebab-case keys and recommended performance improvements to avoid unnecessary string allocations during TOML encoding and decoding.
JesseZhuang
force-pushed
the
jessezhuang/query-output-toml
branch
from
1d4486b to
9a03166
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/util/util.go (1)
181-190: ⚡ Quick winWrap TOML encoder failures before returning.
pelletier.NewEncoder(...).Encodeis a library boundary, so returning the raw error drops the stack trace expected by this repo's error-handling rule. Please wrap it witherrors.Trace(err)orerrors.WrapError(...)before bubbling it up. As per coding guidelines, when an error comes from a third party/library call, wrap it immediately witherrors.Trace(err)orerrors.WrapError(...)to attach a stack trace.Suggested change
func TOMLPrint(cmd *cobra.Command, v interface{}) error { var buf strings.Builder enc := pelletier.NewEncoder(&buf) enc.SetArraysMultiline(true) enc.SetIndentTables(true) if err := enc.Encode(v); err != nil { - return err + return errors.Trace(err) } cmd.Printf("%s", buf.String()) return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/util/util.go` around lines 181 - 190, The TOMLPrint function returns raw errors from the pelletier encoder (enc.Encode) which loses stack traces; update the error return to wrap the encoder error with the repo's error helpers (e.g., return errors.Trace(err) or return errors.WrapError(err, "encode toml") ) right where enc.Encode(v) is called so any library error is immediately annotated with a stack trace before being bubbled up from TOMLPrint.cmd/cdc/cli/cli_changefeed_query.go (1)
233-258: ⚡ Quick winWrap the config round-trip errors as well.
The TOML/JSON encode/decode calls here all return raw library errors, which drops stack traces on a user-visible CLI path. Please wrap each failure with
errors.Trace(err)before returning. As per coding guidelines, when an error comes from a third party/library call, wrap it immediately witherrors.Trace(err)orerrors.WrapError(...)to attach a stack trace.Suggested change
func configToMap(cfg *v2.ReplicaConfig, format util.OutputFormat) (map[string]interface{}, error) { if cfg == nil { return nil, nil } if format == util.OutputFormatTOML { internalCfg := cfg.ToInternalReplicaConfig() var buf bytes.Buffer if err := toml.NewEncoder(&buf).Encode(internalCfg); err != nil { - return nil, err + return nil, errors.Trace(err) } var m map[string]interface{} if _, err := toml.NewDecoder(&buf).Decode(&m); err != nil { - return nil, err + return nil, errors.Trace(err) } return m, nil } data, err := json.Marshal(cfg) if err != nil { - return nil, err + return nil, errors.Trace(err) } var m map[string]interface{} if err := json.Unmarshal(data, &m); err != nil { - return nil, err + return nil, errors.Trace(err) } return m, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/cdc/cli/cli_changefeed_query.go` around lines 233 - 258, In configToMap, wrap all third-party encode/decode errors with a stack-tracing wrapper before returning: replace raw returns from toml.NewEncoder(&buf).Encode(...), toml.NewDecoder(&buf).Decode(...), json.Marshal(cfg), and json.Unmarshal(data, &m) so each error is returned as errors.Trace(err) (or errors.WrapError(...)) instead of the raw err; this ensures all failures in configToMap have stack traces while keeping the existing control flow and return values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/cdc/cli/cli_changefeed_query.go`:
- Around line 233-258: In configToMap, wrap all third-party encode/decode errors
with a stack-tracing wrapper before returning: replace raw returns from
toml.NewEncoder(&buf).Encode(...), toml.NewDecoder(&buf).Decode(...),
json.Marshal(cfg), and json.Unmarshal(data, &m) so each error is returned as
errors.Trace(err) (or errors.WrapError(...)) instead of the raw err; this
ensures all failures in configToMap have stack traces while keeping the existing
control flow and return values.
In `@cmd/util/util.go`:
- Around line 181-190: The TOMLPrint function returns raw errors from the
pelletier encoder (enc.Encode) which loses stack traces; update the error return
to wrap the encoder error with the repo's error helpers (e.g., return
errors.Trace(err) or return errors.WrapError(err, "encode toml") ) right where
enc.Encode(v) is called so any library error is immediately annotated with a
stack trace before being bubbled up from TOMLPrint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5379bfa6-f18b-4d0a-bf7f-c9fc210d45f5
📒 Files selected for processing (5)
cmd/cdc/cli/cli_changefeed_query.gocmd/cdc/cli/cli_changefeed_query_test.gocmd/util/util.gogo.modpkg/api/util.go
Add `--output json|toml` flag to `cdc cli changefeed query`, keeping
JSON as the default. TOML output uses kebab-case keys, human-readable
durations ("10m0s" instead of nanoseconds), and readable timestamps.
The TOML conversion happens entirely in the CLI layer without adding
TOML struct tags to the API model (api/v2/model.go). This is achieved
via two-stage encoding: BurntSushi/toml encodes the internal config
(which already has toml tags) into a map, then pelletier/go-toml/v2
encodes the outer CLI struct with formatting control.
Issue: pingcap#4936
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JesseZhuang
force-pushed
the
jessezhuang/query-output-toml
branch
from
9a03166 to
c7602c0
Compare
ti-chi-bot
Bot
added
contribution
|
Welcome @JesseZhuang! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
2 similar comments
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
|
Could you add a integration test? |
|
I want to clarify the intended contract of If this PR is only adding a TOML format for human-readable inspection, the current direction looks reasonable to me. But if this output is meant to become the foundation for a later “copy from The full query result contains runtime/status fields such as We need to clarify whether |
|
Thanks for the thoughtful question. Let me clarify the design intent: This PR (PR 1) is only adding TOML as an alternative display format — full parity with the existing JSON output for human-readable inspection. In subsequent PRs, That said, the existing code already rejects runtime/status fields in config input via strict TOML decoding. The // cmd/util/util.go:52-68
if undecoded := metaData.Undecoded(); len(undecoded) > 0 {
var b strings.Builder
hasUnknownConfigSize := 0
for _, item := range undecoded {
if hasIgnoreItem(item) {
continue
}
if hasUnknownConfigSize > 0 {
b.WriteString(", ")
}
b.WriteString(item.String())
hasUnknownConfigSize++
}
if hasUnknownConfigSize > 0 {
err = errors.Errorf("component %s's config file %s contained unknown configuration options: %s",
component, path, b.String())
}
}Since the input struct ( So the contract is:
|
|
|
ti-chi-bot
Bot
added
size/XXL
|
Updated integration test in the second commit. The test (
Local validationAll 28 checks pass locally on macOS arm64 (M4 Pro). The TiKV gRPC issue I mentioned earlier turned out to be caused by macOS Application Firewall blocking TiKV — once allowed through System Settings, the cluster bootstraps fine. I verified by running the integration tests on mac. The docker route had an End of Life centos 7 container which was not available from the official repo. What is the recommended way to run integration tests locally? My own integration tests I mentioned earlier were run against a local tidb k8s cluster I started on my mac.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@tests/integration_tests/cli_query_toml/main.go`:
- Around line 68-84: The error returns in readTOML and readJSON currently wrap
third-party errors directly with fmt.Errorf; update both functions to wrap the
original errors using the project helpers (use errors.Trace(err) or
errors.WrapError(err, "message")) instead of fmt.Errorf so stack traces are
attached — for example, when toml.DecodeFile returns an error in readTOML and
when os.ReadFile or json.Unmarshal return errors in readJSON, pass the original
err into errors.Trace or errors.WrapError and return that wrapped error with the
same contextual message.
- Around line 150-181: The comparison currently only validates nested fields
when both type assertions succeed (jOk && tOk), allowing missing or
differently-typed sections to pass silently; update the logic around
jConfig/tConfig, jMounter/tMounter and jFilter/tFilter to explicitly detect and
return an error when one side is missing or has the wrong type (e.g., if jOk !=
tOk or jmOk != tmOk or jfOk != tfOk) with clear messages like "config missing or
type mismatch", "config.mounter missing or type mismatch", and "config.filter
missing or type mismatch" while keeping the existing field-level comparisons
(num(...), toStringSlice(...)) intact so absent sections fail fast instead of
being treated as equal.
In `@tests/integration_tests/cli_query_toml/run.sh`:
- Line 152: The shell invocation currently uses the globbed/expanded form run $*
which can split arguments with spaces; update the script so the test runner call
uses the positional-parameter expansion that preserves argument boundaries by
replacing the use of $* with "$@" (i.e., change the invocation containing the
token run $* to use run "$@" so quoted arguments are passed intact).
- Around line 125-133: The test currently only inspects stderr text for the
invalid-format case using bad_output but doesn't check the command's exit
status; update the block that runs cdc_cli_changefeed query -c "cf-default"
--output yaml to capture the command's exit code (e.g., immediately after
running and redirecting stderr into bad_output) and assert that the exit code is
non-zero in addition to checking stderr content; refer to the variables/command
used (bad_output and the cdc_cli_changefeed query invocation) and fail the test
if the exit code is zero or the stderr doesn't match the expected
invalid/unknown/unsupported text.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72e330d4-3c73-4f6d-99be-36cdcc4af55b
📒 Files selected for processing (10)
cmd/cdc/cli/cli_changefeed_query.gocmd/cdc/cli/cli_changefeed_query_test.gocmd/util/util.gogo.modpkg/api/util.gotests/integration_tests/cli_query_toml/conf/large_filter.tomltests/integration_tests/cli_query_toml/conf/overrides.tomltests/integration_tests/cli_query_toml/main.gotests/integration_tests/cli_query_toml/run.shtests/integration_tests/run_light_it_in_ci.sh
✅ Files skipped from review due to trivial changes (3)
- tests/integration_tests/cli_query_toml/conf/large_filter.toml
- tests/integration_tests/run_light_it_in_ci.sh
- tests/integration_tests/cli_query_toml/conf/overrides.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- go.mod
- pkg/api/util.go
- cmd/cdc/cli/cli_changefeed_query.go
- cmd/cdc/cli/cli_changefeed_query_test.go
- cmd/util/util.go
|
can you help to add a type label on #4936? looks like one check is failing on this pr because of that. |
JesseZhuang
force-pushed
the
jessezhuang/query-output-toml
branch
from
4785fa1 to
d1260ec
Compare
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 `@tests/integration_tests/cli_query_toml/main.go`:
- Around line 53-55: The code currently ignores fmt.Sscanf's error when parsing
the CLI expected count causing invalid input to silently become 0; change the
parsing of os.Args[3] (used as expected) to validate the conversion (e.g., use
strconv.Atoi or capture fmt.Sscanf's returned n and err), and on parse failure
emit a clear error and exit non-zero instead of proceeding to call
checkFilterRules; update the call site around fmt.Sscanf/os.Args[3] to check the
error and only call checkFilterRules(os.Args[2], expected) when parsing
succeeds.
In `@tests/integration_tests/cli_query_toml/run.sh`:
- Line 3: The test script uses "set -eu" which allows upstream command failures
(e.g., `cdc_cli_changefeed query` piped into `grep`) to be masked; update the
shell options in run.sh by adding pipefail (e.g., change the invocation to
include -o pipefail or use set -euo pipefail) so any failure in a piped pipeline
(such as the `cdc_cli_changefeed query | grep ...` commands) causes the script
to exit with error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 529d481d-40fd-4087-aa97-2a961d241bfb
📒 Files selected for processing (6)
tests/integration_tests/cli_query_toml/conf/large_filter.tomltests/integration_tests/cli_query_toml/conf/overrides.tomltests/integration_tests/cli_query_toml/conf/realistic.tomltests/integration_tests/cli_query_toml/main.gotests/integration_tests/cli_query_toml/run.shtests/integration_tests/run_light_it_in_ci.sh
✅ Files skipped from review due to trivial changes (4)
- tests/integration_tests/cli_query_toml/conf/large_filter.toml
- tests/integration_tests/cli_query_toml/conf/overrides.toml
- tests/integration_tests/run_light_it_in_ci.sh
- tests/integration_tests/cli_query_toml/conf/realistic.toml
Add cli_query_toml integration test that verifies: - TOML output is valid and parseable - Kebab-case keys (no snake_case leakage) - Config section structure ([config], [config.filter], etc.) - Human-readable durations and timestamps - JSON/TOML field correspondence - Config overrides reflected correctly - Large filter array formatting - Password redaction in realistic sink URI - TOML indentation (config fields, nested headers, nested fields) - Single-element and multi-element array block format - Simplified query TOML output - Invalid format error handling - Short flag (-o toml) support Uses a Go helper program for structured TOML/JSON validation (BurntSushi/toml, already a project dependency). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
JesseZhuang
force-pushed
the
jessezhuang/query-output-toml
branch
from
d1260ec to
b063815
Compare
2 participants
What problem does this PR solve?
cdc cli changefeed create --configconsumes TOML, butcdc cli changefeed queryonly returns JSON. This asymmetry makes changefeed migration, config review, and version control more manual than necessary.Issue Number: ref #4936
What is changed and how it works?
Add
--output json|tomlflag tocdc cli changefeed query, keeping JSON as the default.TOML output uses kebab-case keys, human-readable durations (
10m0sinstead of nanoseconds), and readable timestamps (2026-04-18 09:35:00.139).The TOML conversion happens entirely in the CLI layer without adding TOML struct tags to
api/v2/model.go. This is achieved via two-stage encoding:config.ReplicaConfig(which already has toml tags) into amap[string]interface{}— this produces correct kebab-case keys and human-readable duration strings.cfMetaTOML) withSetIndentTables/SetArraysMultilinefor formatting. TheConfigfield isinterface{}holding the map from step 1.This keeps the API model as a pure JSON HTTP contract per feedback in #4936.
This is PR 1 of 3 for #4936. PR 2 will add
--diff-default, PR 3 will add config file enhancements.Check List
Tests
Integration tests
Built from this branch and deployed to a local TiDB-on-k3d cluster (TiCDC Classic kernel, v8.5.6 base). 27 pytest-based integration tests, all passing:
[config]section structure, durations as'10m0s', readable timestamps, single-quoted strings, JSON↔TOML field correspondence, overrides changefeed\Ncsv null in both formats[config.*]fields 4-space indentedintegration-tests-pr1-20260502.zip
Questions
Will it cause performance regression or break compatibility?
No. JSON output is unchanged (default). TOML output is a new opt-in flag. No changes to the API model or server behavior.
Do you need to update user documentation, design documentation or monitoring documentation?
CLI help text is updated via the new
--outputflag. No other documentation changes needed.Release note
Summary by CodeRabbit
New Features
changefeed querygains an--output/-ooption to emit JSON or TOML (full or simplified); TOML uses kebab-case keys and human-readable durations/timestamps.Tests
Chores