feat(workspace): fast inline results for workspace query, --full for complete CSV (0.59.0) - #406
Conversation
…for complete CSV (0.59.0)
`workspace query` materialized a CSV file via the warehouse UNLOAD path
(GET .../export?fileType=csv) for every query, paying a slow
warehouse -> object-storage -> download round-trip even for tiny result
sets. The default now reads results inline via the Query Service
GET /api/v1/queries/{job}/{stmt}/results endpoint (JSON columns+rows, no
file materialization) -- ~2.5x faster on a 25-row table, more on larger
overhead-dominated queries.
- Each statement carries structured columns + rows (+ row_count,
total_rows, truncated) and a synthesized csv_data, so the CLI preview,
web UI table, and any --json consumer keep working unchanged.
- --limit N (default 500) caps the fast path; it pages by offset and
marks the result truncated when the warehouse has more. The /results
endpoint enforces 100 <= pageSize <= 100000, so pageSize is a fixed
valid value and --limit only trims locally (a small --limit no longer 400s).
- --full opts back into the complete CSV export (slower, uncapped) for
bulk extraction.
- serve /workspaces/{p}/{w}/query defaults to full=True to preserve the
web UI's complete-CSV download until the frontend paginates.
Verified live against the demo "shop" project (Snowflake workspace,
25-row table): structured output, NULL handling, --limit truncation,
--full export, and the ~2.5x speedup.
padak
left a comment
There was a problem hiding this comment.
Review of #406 — feat(workspace): fast inline results for workspace query, --full for complete CSV (0.59.0)
Generated by
kbagent-pr-reviewersubagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed viamake check, not duplicated here.
Summary
This PR replaces the workspace query result-fetch strategy: instead of always materialising a CSV file through the warehouse UNLOAD path, the default now reads results inline via the Query Service /api/v1/queries/{job}/{stmt}/results endpoint (paginated, capped at --limit rows, default 500). A --full flag reverts to the old CSV-export path for complete result sets. The JSON output shape is additive (new columns, rows, row_count, total_rows, truncated fields; csv_data is synthesised and preserved for backward compat). All three layers are touched correctly, documentation surfaces are updated, make check is green at 3909 tests. One NON-BLOCKING logic gap was found in the truncation signal, and two NON-BLOCKING documentation gaps remain in keboola-expert.md. APPROVE.
Verdict
- Verdict: APPROVE
- Blocking findings: 0
- Non-blocking findings: 3
- Nits: 2
Blocking findings
(none)
Non-blocking findings
[NB-1] src/keboola_agent_cli/services/workspace_service.py:219 — truncated is incorrectly False when numberOfRows is absent from API response and rows hit the limit
The truncation signal is truncated = total_rows is not None and total_rows > len(rows). If the Query Service does not return numberOfRows in its payload (a defensive case), total_rows stays None and truncated is False — even though we stopped collecting because we hit --limit, not because the warehouse ran out of rows. A caller reading truncated: false would believe they received the complete result set when they may not have.
Fix: change the condition to truncated = (total_rows is not None and total_rows > len(rows)) or (len(collected) >= limit and len(rows) == limit) — flag truncation whenever we filled the limit, regardless of whether numberOfRows was returned. Add a test case where the API returns no numberOfRows and len(collected) == limit.
[NB-2] plugins/kbagent/agents/keboola-expert.md:103 and :344-359 — keboola-expert.md §2 matrix and §4.4 workflow do not mention the new default cap
The §2 "Ad-hoc SQL / row-count / type audit" matrix row and the §4.4 "Workspace-based SQL debugging" workflow example both show workspace query --sql "..." without any mention of --full or the 500-row default cap introduced in this PR. The workflow note at line 358 explicitly says to use this for "ROW COUNT COMPARISONS between branches" — a query returning a 501-row table will silently return 500 rows as truncated: true, but an agent that ignores the truncated flag (or just looks at row_count) will draw a wrong comparison.
Per CONTRIBUTING.md §2 the matrix carries the (X.Y.Z+) version floor and is updated when behaviour changes. Fix: add a note to the "Ad-hoc SQL" matrix row and the §4.4 example, e.g. workspace query --sql "SELECT COUNT(*) ..." (aggregate queries are fine); add a one-liner to §3 "Workspace" group: **workspace query default cap 500 rows (0.59.0+)**: results capped at \--limit` (default 500) on the fast path; use `--full` for complete result sets, check `truncated` before trusting row counts.`
[NB-3] tests/test_workspace_service.py — four new execute_query test cases do not assert mock_client.close.call_count == 2
The existing test_execute_query_success (line 1008) and all other happy-path service tests assert mock_client.close.call_count == 2 (one call in _resolve_branch_id, one in the finally block of execute_query). The four new tests test_execute_query_full_uses_csv_export, test_execute_query_inline_pagination, test_execute_query_small_limit_keeps_valid_page_size, and test_execute_query_inline_fetch_fails_gracefully omit this assertion. Per CONTRIBUTING.md > Testing Guidelines: "Verify client.close() is called." A missing close() would leak an httpx connection but no test would catch it.
Fix: add assert mock_client.close.call_count == 2 at the end of each of the four new happy-path / pagination tests.
Nits
-
[NIT-1]src/keboola_agent_cli/services/workspace_service.py:216— whentotal_rowsis exactly equal toQUERY_RESULTS_PAGE_SIZE(e.g. exactly 500 rows in the warehouse, limit=600, page_size=500), the pagination loop will make a second API call tooffset=500before discovering the empty page and breaking. This is not a bug — it's a single wasted round-trip — but could be avoided by checkingtotal_rows is not None and offset + len(page_rows) >= total_rowsas an early break condition. -
[NIT-2]src/keboola_agent_cli/constants.py:339—QUERY_RESULTS_PAGE_SIZEandQUERY_RESULTS_DEFAULT_LIMITare currently both 500, which may create the impression they are the same concept. A brief inline comment (or at least a divergent example value in the docstring) would help the next reader understand thatpage_sizeis an API wire constraint (100..100000) andlimitis a user-facing row cap that can be independently tuned.
Verification log
gh pr view 406 --json title,body,files,additions,deletions,baseRefName,headRefName,labels,state→ 21 files, +794/-60, state OPEN, conventionalfeat(workspace):prefix ✓git rev-parse --abbrev-ref HEADin worktree →claude/upbeat-payne-1ed2e5matches PR branch ✓- Layer violation checks (typer/click in services, httpx in commands, formatter in clients) → empty ✓
grep -E '^\+' diff | grep -E 'bare except'→ empty ✓grep -E '^\+.*error_code\s*=\s*"[A-Z_]+"' diff→ one hit in test fixture only (not production code) ✓- Token discipline:
token="901-55555-fakeTestTokenDoNotUseXXXXXXXX"is the canonical test token ✓ - Plugin synchronization map: no new commands added; only flags added to existing
workspace.query; noOPERATION_REGISTRYentry needed (command already registered atsrc/keboola_agent_cli/permissions.py:107) ✓ CLAUDE.md## All CLI Commands→ updated with[--full] [--limit N]at correct line ✓src/keboola_agent_cli/commands/context.pyAGENT_CONTEXT→ updated ✓plugins/kbagent/skills/kbagent/references/commands-reference.md→ updated ✓plugins/kbagent/skills/kbagent/references/gotchas.md→ new section(since v0.59.0)with version tag ✓plugins/kbagent/skills/kbagent/references/workspace-workflow.md→ new section added ✓plugins/kbagent/agents/keboola-expert.md→gotchas.mdcross-reference exists; §2 matrix row for workspace unchanged; §3 has no new workspace gotcha → gap flagged as NB-2src/keboola_agent_cli/server/routers/workspaces.py→WorkspaceQuery.full/limitadded;full=Truedefault preserves web UI shape ✓make check→ 3909 passed, 8 skipped, 124 deselected, 15 warnings (coroutine warnings from pre-existing MCP tests) in 83.48s, exit 0 ✓- Backward compatibility:
csv_datasynthesised on fast path;full=Truedefault on serve preserves old web UI shape;rows_affectedsource nowstmt.get("numberOfRows", stmt.get("resultRows", 0))— graceful fallback for older API responses ✓ - Pagination logic review: no infinite loop possible (while exits at
len(collected) >= limit); truncation logic has one edge case whennumberOfRowsabsent (flagged as NB-1) ✓ - E2E tests:
_test_workspace_queryextended to cover both fast path (columns/rows/csv_data asserted) and--fullpath ✓ - Behavior reproduction: could not run live against real project (no credentials in this session); PR description includes a verified live measurement (
~2.5x faster) and the--limit 5regression story matches the regression test attests/test_workspace_service.py:1135✓
Open questions for the author
(none)
…path
Devin Review + kbagent-pr-reviewer follow-ups (all non-blocking):
- csv_data complex types: VARIANT/ARRAY/OBJECT (Snowflake) and STRUCT/ARRAY
(BigQuery) cells that arrive as native dict/list are now serialized as
compact JSON ({"k":"v"}) in the synthesized csv_data instead of Python repr
({'k': 'v'}), matching the warehouse CSV export. New _csv_cell helper.
- truncated robustness: when the Query Service omits numberOfRows, fall back to
how the pagination loop ended -- stopping at the --limit cap with a full last
page (not exhausted) now reports truncated=True instead of silently False.
- --limit validation: reject a non-positive --limit. min=1 on the Typer option
(CLI usage error) and Field(ge=1) on the REST WorkspaceQuery model, so a
zero/negative limit no longer silently yields an empty result.
- docs: keboola-expert.md tool-selection matrix + workspace SQL-debugging
workflow now warn that `workspace query` is capped at --limit (default 500) --
use COUNT(*) for counts, check truncated/total_rows, --full for the complete
set. gotchas.md notes the complex-type JSON serialization.
- tests: + close.call_count assertions on the new service tests, + complex-type
csv, truncated-without-count, and non-positive --limit coverage.
The client.py file-size finding (3232 LOC > 2000 hard cap) is pre-existing tech
debt and out of scope for this feature PR; tracked as a follow-up split.
…tion - NIT-1: break the /results pagination loop as soon as the accumulated offset reaches the reported total_rows on a page boundary, instead of spending a round-trip on the empty next page (e.g. total == a multiple of the page size with --limit larger than total). - NIT-2: clarify in constants.py that QUERY_RESULTS_PAGE_SIZE (API wire constraint, 100..100000) and QUERY_RESULTS_DEFAULT_LIMIT (user-facing row cap) are distinct concepts that merely share the value 500. - test: the early break makes no wasted round-trip on a page boundary.
Why
kbagent workspace querymaterialized a CSV file via the warehouse UNLOAD path (GET .../export?fileType=csv) for every query, paying a slowwarehouse -> object-storage -> downloadround-trip even for tiny result sets. For an interactive SQL-debugging tool this is the wrong default.What changed
The default now reads results inline via the Query Service
GET /api/v1/queries/{job}/{stmt}/resultsendpoint (JSONcolumns+rows, no file materialization). Measured ~2.5x faster live on a 25-row table (inline ~3.1s vs--full~7.9s); the gap is the fixed file-export overhead and grows with volume.columns+rows(plusrow_count,total_rows,truncated) and a synthesizedcsv_data, so the CLI preview, web UI table, and any--jsonconsumer keep working unchanged (no breaking shape change).--limit N(default 500) caps the fast path; it pages byoffsetand flags the statementtruncatedwhen the warehouse has more rows. The CLI printsShowing first N of TOTAL rows. Use --full….--fullopts back into the complete CSV export (slower, uncapped) for bulk extraction.kbagent serve/workspaces/{p}/{w}/queryREST endpoint defaults tofull=Trueto preserve the web UI's complete-CSV "Download" until the frontend learns to paginate (REST clients can passfull=false).API floor caught during live testing
The
/resultsendpoint enforces100 <= pageSize <= 100000(a smallerpageSize400s withInvalid pageSize parameter, must be between 100 and 100000). DerivingpageSizefrom--limitbroke--limit 5. Fixed:pageSizeis a fixed valid value and--limitonly trims the accumulated rows locally. Locked in by a regression test.Layers touched (3-layer design)
client.pyget_query_results()on/results;export_query_resultskept for--fullservices/workspace_service.pyexecute_query(full, limit);_collect_inline_results(paginate+trim),_rows_to_csvoutput.py--fullcommands/workspace.py--full,--limitflagsserver/routers/workspaces.pyWorkspaceQuery.full/limit(defaultfull=True)Tests & docs
/resultspagination params), service (inline path, pagination, truncation, small-limit regression,--full, graceful failures), CLI (--full/--limitforwarding), output (table vs CSV fallback).make checkgreen (3909 passed)._test_workspace_querynow exercises both the fast and--fullpaths.CLAUDE.md,context.py(AGENT_CONTEXT),commands-reference.md,gotchas.md(since v0.59.0, incl. the pageSize floor), and a new "Fast inline results vs--full— mind the result-set volume" section inworkspace-workflow.md.make version-sync).Live verification (demo "shop", Snowflake workspace, 25-row table)
Structured output + NULL handling,
--limit 5truncation (row_count:5,total_rows:25,truncated:true), human-readable Rich table + hint,--fullcomplete 26-row CSV, and the ~2.5x speedup.