VDiff: add --no-samples option to vdiff show

[pedroalb]

Description

vdiff show always reads the per-table diff report from _vt.vdiff_table.report and returns it in the response, regardless of client --format. The report's row-sample arrays (MismatchedRowsSample, ExtraRowsSourceSample, ExtraRowsTargetSample) carry actual sampled row data, including large blob/JSON columns, and because vdiff show fans out to every target shard's primary and aggregates, the response can exceed gRPC message limits on large diffs. Callers that only need progress, counts and the has_mismatch flag still pay the full cost of the samples.

This PR adds an opt-in no_samples option, threaded from a new client flag --no-samples through VDiffShowRequest.no_samples (vtctld) to VDiffReportOptions.no_samples (tablet). When set, the tablet's summary query strips the sample arrays from the report with JSON_REMOVE while preserving the scalar counters, so the large sampled rows are neither read into the response nor sent to vtctld, and the reported counts stay accurate.

Fixes #20869.

Behavior

• Default (--no-samples unset): unchanged — the full report is returned as today.
• With --no-samples: the response carries all summary fields (state, table rows, rows compared, started/completed timestamps, has_mismatch) and the report's scalar counters (ProcessedRows, MatchingRows, MismatchedRows, ExtraRowsSource, ExtraRowsTarget), but with the sampled-row arrays removed.
• vdiff create --wait uses no_samples for its internal progress polls, so the wait loop no longer transfers the sampled rows on every interval and can't be killed by the gRPC limit on large diffs. Text output is unchanged (the samples are never rendered in the wait summary); with --format json the per-interval output no longer includes the sampled rows (they remain available via vdiff show --verbose once the diff completes).

Design note: why strip samples rather than blank the whole report

The first revision of this PR replaced the whole report with a literal '{}'. Review (thanks @chatgpt-codex-connector, @Copilot) surfaced two problems with that:

1. It produced misleading counts. BuildSummary (go/vt/vtctl/workflow/vdiff.go) parses the per-table/per-shard ProcessedRows, MatchingRows, MismatchedRows, and ExtraRows* counters out of the report JSON. Blanking the report to {} made those parse as zero, so a summary could report HasMismatch: true alongside MismatchedRows: 0 and ProcessedRows: 0 — internally inconsistent output.
2. It changed NULL semantics. vdt.report as report is NULL when the LEFT JOIN produces no vdiff_table row; a literal '{}' is not, silently changing behavior for that case.

Stripping only the sample arrays with JSON_REMOVE(vdt.report, '$.MismatchedRowsSample', '$.ExtraRowsSourceSample', '$.ExtraRowsTargetSample') fixes both: the counters that BuildSummary needs are preserved so counts stay accurate, and JSON_REMOVE(NULL, ...) returns NULL so the no-row case matches the plain-column behavior. It also removes exactly the part that causes the gRPC-size problem (the sampled rows are the only unbounded, blob-carrying portion of the report), so the original goal is still met.

Note this means the report column is still read by MySQL to evaluate JSON_REMOVE; what is avoided is transferring the large sampled rows into the response and on to vtctld. The diff-time coordinator memory cost is a separate concern tracked in #19735.

Implementation notes

• The summary query is composed from shared vdiffSummaryCols and vdiffSummaryFrom constants; sqlVDiffSummary appends vdt.report as report and sqlVDiffSummaryNoSamples appends the JSON_REMOVE(...) expression. The two variants can therefore only differ in the report select-expression — every other column stays identical, so no-samples can never silently drop other summary data.
• getVDiffSummary takes the request's *VDiffReportOptions and picks the query via vdiffSummaryQuery(reportOpts.GetNoSamples()).

Testing

• TestVDiffSummaryQuery (tabletmanager/vdiff) asserts structurally (not by brittle full-string match) that: both variants keep the %a bind placeholders; the full variant returns the report as-is and does not use JSON_REMOVE; the no-samples variant strips exactly the three sample arrays; and the two variants are identical after swapping the report select-expression, so no other column or clause can silently differ.
• TestPerformVDiffAction show-by-uuid cases assert that handleShowAction runs the JSON_REMOVE query when no_samples is set and the full-report query otherwise (exercising the real request path, not just the query strings).
• TestVDiffShow (workflow) asserts that VDiffShow forwards no_samples into the tablet VDiffRequest sent to every target primary, for both flag values.
• TestShowNoSamplesFlag guards the --no-samples flag registration (default and usage).
• The JSON_REMOVE semantics were verified on real MySQL 8.0.46 and 5.7.44, including the full summary query against a populated _vt.vdiff/vdiff_table schema: samples stripped, all scalar counters preserved, and the LEFT JOIN-with-no-row case returns a NULL report on both versions.
• Proto bindings regenerated via make proto. go build of the affected packages, go vet, gofmt, and golangci-lint (via pre-commit) are clean.

Backport rationale

This is a small, opt-in, backward-compatible change (default behavior is unchanged) that fixes a real operational failure: vdiff show can currently fail outright when the aggregated report exceeds gRPC message limits on large diffs, leaving operators with no way to read the summary or mismatch state. Because that failure blocks monitoring of in-flight VDiffs on affected keyspaces, it is worth backporting to the currently supported release branches (release-23.0, release-24.0) so operators on those releases can retrieve summaries for large diffs without upgrading. The change is confined to the vdiff show path and touches no default behavior, so backport risk is low.

AI Disclosure

This change was co-authored with Claude Code, which helped with implementation and testing.

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 14 changed files in this pull request and generated no new comments.

Files not reviewed (3)

• go/vt/proto/tabletmanagerdata/tabletmanagerdata.pb.go: Generated file
• go/vt/proto/tabletmanagerdata/tabletmanagerdata_vtproto.pb.go: Generated file
• go/vt/proto/vtctldata/vtctldata.pb.go: Generated file

Suppressed comments (3)

go/vt/vtctl/workflow/vdiff.go:573

• This always populates Options/ReportOptions even when OnlySummary is false (the default). If downstream code treats nil options differently from an empty options message (e.g., for applying defaults), this can unintentionally change behavior for callers that did not set only_summary. Consider only setting Options when req.GetOnlySummary() is true; otherwise leave it nil to preserve prior request shape.

		Options: &tabletmanagerdatapb.VDiffOptions{
			ReportOptions: &tabletmanagerdatapb.VDiffReportOptions{
				OnlySummary: req.GetOnlySummary(),
			},
		},

go/vt/vttablet/tabletmanager/vdiff/action.go:134

• getVDiffSummary only needs a boolean, but it now depends on the full *VDiffReportOptions message. This increases coupling and makes the function signature more complex than necessary. Consider changing the parameter to onlySummary bool and having the caller pass req.GetOptions().GetReportOptions().GetOnlySummary(); this keeps the function focused and simplifies future extension/testing.

func (vde *Engine) getVDiffSummary(vdiffID int64, dbClient binlogplayer.DBClient, reportOpts *tabletmanagerdatapb.VDiffReportOptions) (*query.QueryResult, error) {

go/vt/vttablet/tabletmanager/vdiff/action.go:138

• getVDiffSummary only needs a boolean, but it now depends on the full *VDiffReportOptions message. This increases coupling and makes the function signature more complex than necessary. Consider changing the parameter to onlySummary bool and having the caller pass req.GetOptions().GetReportOptions().GetOnlySummary(); this keeps the function focused and simplifies future extension/testing.

	query, err := sqlparser.ParseAndBind(vdiffSummaryQuery(reportOpts.GetOnlySummary()), sqltypes.Int64BindVariable(vdiffID), sqltypes.StringBindVariable(vde.dbName))

[mattlord]

Nit, but summary feels very overloaded here to the point that it loses all meaning. Maybe we could at least call sqlVDiffSummaryOnly something like sqlVDiffSummaryMinimal or sqlVDiffSummaryWithoutSamples? Or maybe generally we use Full and Minimal like we have in some code here.

[mattlord]

This LGTM, but I think our current overloaded use of summary is a bit unintuitive and ambiguous. I think that the client option/flag could be --no-samples versus --only-summary, or even --minimal or something. What do you think? I would not call the flag portion a blocker, but I do think we can at least improve this aspect in the code throughout as my inline comment talks to.

[pedroalb]

This LGTM, but I think our current overloaded use of summary is a bit unintuitive and ambiguous. I think that the client option/flag could be --no-samples versus --only-summary, or even --minimal or something. What do you think? I would not call the flag portion a blocker, but I do think we can at least improve this aspect in the code throughout as my inline comment talks to.

100% @mattlord, thanks for the review. I think --no-samples fits better. Let me update this + double check the failed CI tests. I should have this fixed by the end of tomorrow morning European time.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 16 changed files in this pull request and generated no new comments.

Files not reviewed (3)

• go/vt/proto/tabletmanagerdata/tabletmanagerdata.pb.go: Generated file
• go/vt/proto/tabletmanagerdata/tabletmanagerdata_vtproto.pb.go: Generated file
• go/vt/proto/vtctldata/vtctldata.pb.go: Generated file

[mattlord]

Non-blocking: I think we should probably restore the release-note entry in changelog/25.0/25.0.0/summary.md since this is a new feature rather than a bug fix. It seems like commit 6614ddf removed the --only-summary entry during the rename without adding an updated --no-samples version, so the new flag and intentional vdiff create --wait --format json output change are no longer documented for operators. Otherwise, this looks good to me. It's also a minor thing so it doesn't HAVE to be in the release summary. Thanks again, @pedroalb !

If you merge in upstream main then the static code checks workflow should pass.

[pedroalb]

Non-blocking: I think we should probably restore the release-note entry in changelog/25.0/25.0.0/summary.md since this is a new feature rather than a bug fix. It seems like commit 6614ddf removed the --only-summary entry during the rename without adding an updated --no-samples version, so the new flag and intentional vdiff create --wait --format json output change are no longer documented for operators. Otherwise, this looks good to me. It's also a minor thing so it doesn't HAVE to be in the release summary. Thanks again, @pedroalb !

If you merge in upstream main then the static code checks workflow should pass.

Done - restored the release note and merged in upstream main. Thanks for the review, @mattlord!

"If you merge in upstream main then the static code checks workflow should pass."

unfortunately the static code checks are still failing

[Copilot]

Pull request overview

Copilot reviewed 12 out of 16 changed files in this pull request and generated no new comments.

Files not reviewed (3)

• go/vt/proto/tabletmanagerdata/tabletmanagerdata.pb.go: Generated file
• go/vt/proto/tabletmanagerdata/tabletmanagerdata_vtproto.pb.go: Generated file
• go/vt/proto/vtctldata/vtctldata.pb.go: Generated file