[INIT-6549] Add client-side synchronous snapshot queries to the Flink CLI - #3466
[INIT-6549] Add client-side synchronous snapshot queries to the Flink CLI#3466Yiyu Tian (yiyutian1) wants to merge 2 commits into
Conversation
|
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Pull request overview
Adds a new client-side “snapshot query” execution path for Flink SQL, including a top-level confluent query command that submits a statement, blocks until it leaves PENDING, drains all result pages, and prints results (table or serialized JSON/YAML), with best-effort stop behavior on interrupts/timeouts.
Changes:
- Introduces
pkg/flink/querywith an await+drain loop for synchronous result collection (including boundedness checks, truncation/incomplete signaling, and wrapped fetch errors). - Adds SQL-type-aware serialization via
StatementResultField.ToSerializedValue()and corresponding unit tests. - Mounts the new
confluent querycommand at the CLI root and tightens the Flink gateway mock to reject malformed stop/update requests consistent with the real gateway.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/test-server/flink_gateway_router.go | Makes the mock gateway stricter by rejecting statement updates that omit SQL text, matching real gateway behavior and catching broken stop paths. |
| pkg/flink/types/statement_traits.go | Adds helpers to read bounded/append-only traits with “known/unknown” signaling. |
| pkg/flink/types/result_fields.go | Extends StatementResultField with ToSerializedValue() for typed JSON/YAML output. |
| pkg/flink/types/result_fields_serialized.go | Implements typed serialization rules (e.g., NULL→null, small ints→numbers, BIGINT/DECIMAL→strings). |
| pkg/flink/types/result_fields_serialized_test.go | Unit tests for typed serialization behavior and JSON round-trip precision expectations. |
| pkg/flink/types/processed_statement.go | Adds STOPPED/DELETING phases for terminal-phase handling. |
| pkg/flink/query/README.md | Documents rationale, drain-loop semantics, and known limitations of the synchronous query path. |
| pkg/flink/query/query.go | Implements query.Run, await, drain, and terminal-phase logic for synchronous snapshot queries. |
| pkg/flink/query/query_test.go | Unit tests for paging, truncation, incomplete detection, boundedness rejection, and error wrapping. |
| internal/query/command.go | Adds the confluent query Cobra command, wiring flags, submit/run/stop flow, and output formatting. |
| internal/command.go | Registers the new root-level query command. |
Suppressed comments (1)
internal/query/command.go:539
- The JSON/YAML envelope is constructed without surfacing
result.Incomplete, so even if the drain loop flags the result as incomplete, serialized output won't include it.
Truncated: result.Truncated,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The stderr warning for a gateway that stopped returning page tokens mid-run was the only signal of a partial result set; a script reading -o json/-o yaml had no way to detect it. Per Copilot review on #3466.
…queries Adds a top-level `confluent query` command that submits a bounded Flink SQL statement, blocks until it finishes, and prints the complete result set, exiting non-zero on failure. Intended for scripts and one-shot queries; the interactive `flink shell` remains the tool for exploring a streaming result. Mounted at the top level rather than under `flink statement`: per discussion with Jim Hughes and Florian Eiden, the verb should not name Flink or the statement resource, since the same one-shot query ergonomics are expected to cover other backends (e.g. Lightning Tables) later without a rename. Output defaults to a self-describing envelope (column schema + rows); `--raw` opts into a bare row array. Values are type-aware — a number serializes as a number and a NULL as null, rather than everything round-tripping as a string. Folded in from post-review fixup commits: - error message now follows repo convention (lowercase, no trailing period, flags in backticks) - ListFlinkComputePools call updated for its current three-arg signature - the JSON/YAML envelope now surfaces Incomplete alongside Truncated - help goldens regenerated for the new command
ee1da79 to
9707001
Compare
|
An automated review pass flagged a few things worth a human glance. These scored moderately-to-highly in initial triage but weren't independently re-verified in a second pass, so please treat them as leads rather than confirmed issues. Suggestion — flag formatting in error/suggestion messages (
This repo's output-formatting convention documents flags as backtick-formatted — these three look like a consistent slip across the new command's error paths. Suggestion — test coverage (
Suggestion — root command registration (
Suggestion — stale doc line (
Suggestion — comment cites the wrong precedent (
Comment generated with the help of an AI agent |
- Backtick-format flag names in error/suggestion strings for consistency with the rest of the repo's output conventions. - Fix the Hidden-gating comment's precedent: unified-stream-manager doesn't exist in this repo; point at the private link ingress endpoint command instead, which is the actual precedent for this pattern. - Correct the README's stale "no token refresh" limitation now that Options.RefreshToken exists, and note that the default 10-minute timeout is on the same order as the dataplane token's lifetime so it rarely matters in practice. - Add internal/query/command_test.go covering buildQueryProperties, printQueryResult, refreshGatewayToken, stopStatement and handleQueryError, raising this package's coverage from 0% to address the SonarQube new-code coverage gate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|


Implements the client-side path for one-shot Flink SQL snapshot queries (INIT-6549 M1, PRD R3): submit → block → drain every page → print → exit non-zero on failure.
Diagram here: here
example use case:
Mounted at the top level (
confluent query), not underflink statement— per discussion with Jim Hughes and Florian Eiden, the same one-shot ergonomics should extend to other backends (e.g. Lightning Tables) later without a rename.-o json/-o yamldefault to a self-describing envelope (schema + rows);--rawgives a bare row array.Verified against real staging. Submit → multi-page drain → exit-code behavior confirmed on a real compute pool. That run found and fixed two defects:
spec.stopped; every stop (interrupt, timeout, unbounded rejection) left the statementRUNNING. Fixed by reading the statement back and flipping the flag on it, likestatement stopdoes. The mock server was more permissive than the real gateway and missed this — it's now strict enough to catch it.INTEGERcame back as"3065"). Now type-aware: numbers as numbers,NULLasnull.Why a separate drain loop instead of the shell's
Store/ResultFetcherpipeline: that pipeline is built for a scrolling viewer and degrades silently in ways that become real bugs for a script reading stdout (row-cap eviction, schema-mismatch rows dropped, "done" inferred from a missing page token without checking phase). This drain loop reports each of those conditions instead of hiding them.Not done: on-prem support, statement cleanup on success,
--unsafe-tracestill dumps row data, no token-refresh-aware retry beyond a best-effort refresh before each call.Open questions: final mount point/verb, output shape (R3 wants a bare array, R10 wants a typed schema — currently reconciled via envelope-by-default +
--raw), timeout behavior (currently stops the statement and exits non-zero; PRD wants graceful degradation).Testing: 14 unit tests over a mocked gateway (multi-page drain, empty-token-while-running,
--max-rowsboundary, unbounded rejection, schema mismatch, cancellation).make lint-cliclean. Integration goldens deferred — the mount point and flags are still in flux pending PM sign-off.🤖 Generated with Claude Code