feat(qwp): scope the SYMBOL dictionary to a single query

[kafka1991]

Tandem PR (server): questdb/questdb#7321 — it carries the protocol change and, because most of this client's real-server coverage runs against a live ServerMain in the questdb repo, the end-to-end and protocol tests for this change. Review the two together.

Why

QWP carries a SYMBOL column as a dictionary (the distinct string values) plus per-row integer codes. The fastest egress consumers — the Python client's to_polars() / to_pandas() dataframe paths — reuse that wire dictionary directly as the pandas/polars Categorical dictionary: the per-row codes are adopted zero-copy, with no re-interning and no per-row remap. That direct reuse is what makes SYMBOL → dataframe maximally efficient.

The catch is that the QWP dictionary is connection-scoped: it accumulates every symbol seen across all queries on a connection and only resets at a soft cap. Because the consumer reuses the dictionary as-is, a result that references few symbols but lands on a connection an earlier high-cardinality query already grew inherits a bloated Categorical — full of categories it never uses. The very zero-copy reuse that makes the path fast is what drags the cross-query bloat in.

The protocol fix (server PR #7321) adds a per-query query_flags field to QUERY_REQUEST whose first bit, QUERY_FLAG_RESET_DICT, asks the server to reset the connection-scoped dictionary before the query streams — keeping the zero-copy speed without the bloat. This PR is the Java client half: it lets a Java caller set that flag per query.

What

QwpQueryClient gains execute(..., boolean resetSymbolDict) overloads. The existing two- and three-argument forms delegate with false, so current callers are unchanged. When the flag is set, the client asks the server to reset the connection-scoped SYMBOL dictionary before the query streams, scoping the dictionary to that query's own symbols. The flag is opt-in and defaults off.

• resolveQueryFlags maps the request to the QUERY_FLAG_RESET_DICT bit, but only when the server advertised the CAP_QUERY_FLAGS capability in SERVER_INFO; against a server without it the flag is silently dropped.
• QwpEgressIoThread appends the query_flags varint trailer to QUERY_REQUEST only when the resolved flags are non-zero, so a baseline frame stays byte-identical on the wire.

Compatibility

• Old server + new client: no capability advertised, so no trailer is sent — unchanged.
• New server + old client: no trailer, server defaults flags to 0 — unchanged.
• New server + new client: the flag is honoured.

Tradeoff

Setting the flag gives up cross-query dictionary reuse for that query: the dictionary and its per-batch delta are rebuilt from scratch, so the query's symbols are re-sent rather than amortized across the connection. That is the intended exchange — a lean, query-scoped Categorical in return for re-sending the query's own symbols. Callers that benefit from connection-scoped accumulation should leave the flag off (the default).

Testing

Most of this client's real-server coverage lives in the questdb repo (it runs against a live ServerMain), so the protocol and end-to-end tests for this change ship in the tandem PR #7321:

• QwpEgressServerInfoTest — the OSS server advertises CAP_QUERY_FLAGS, which is what gates the client appending the trailer.
• QwpEgressRequestDecoderTest — decoding of the optional query_flags trailer: the reset bit, a trailer after a non-empty bind section, multi-byte varint values, and high/sign bits all decode; a baseline frame leaves it 0.
• QwpEgressQueryFlagsResetTest — the decode → dict-reset effect: flag absent keeps the conn dict; flag set clears it and stages the CACHE_RESET; an empty dict suppresses the redundant frame; an unknown flag bit is ignored.
• QwpEgressProcessorStateCacheResetTest — reset-mask boundaries.
• QwpEgressCacheResetWireTest — the end-to-end client↔server CACHE_RESET wire path this builds on, driving this client against a live server.

In this repo, the existing QWP client/egress unit tests still pass and a flag-off frame stays byte-identical on the wire.

[kafka1991]

Code review (level 3 — full pass)

Change-surface map + 10-agent fan-out (incl. fresh-context adversarial & cross-context) with per-finding source verification.

Verdict: Approve — no blocking issues

The change is small, additive, and correctly wired. Existing callers are untouched (the new boolean overloads sit at non-ambiguous positions), the reused QueryRequest holder gets queryFlags written on every submit (no stale leak), failover re-resolves capabilities per attempt against the freshly-published serverInfo, and the wire trailer is correctly gated and byte-identical-when-off. The new test is meaningful and not flaky. No java.util.* on a data path, no zero-GC violations, no Java-8-floor violations. No Critical findings — everything below is advisory.

---

Moderate

M1 — Feature correctness depends entirely on the tandem server PR; confirm two contracts there (cross-repo, fails safe).
This client half resets only the server's dict (via the wire flag). It never resets its own local dict mirror — the diff doesn't touch the decoder. The per-query path only calls decoder.resetQuerySchema() (QwpEgressIoThread.java:323), which clears the schema, not the connection-scoped SYMBOL dict (connDictSize). The client's local dict is reset only by a server CACHE_RESET frame → applyCacheReset (QwpResultBatchDecoder.java:184).

So the feature is coherent only if the server, when honoring QUERY_FLAG_RESET_DICT, also emits a CACHE_RESET before the query's first RESULT_BATCH. If the server reset its dict without CACHE_RESET, the next batch's deltaStart=0 would mismatch the client's non-zero connDictSize and trip the guard at QwpResultBatchDecoder.java:749-752 ("delta symbol dict out of sync") → terminal transport error → failover → onError. That's a clean, fail-safe error (no corruption / UAF), so it's not a client bug. Before merge, confirm in #7321 that:

1. The server defines CAP_QUERY_FLAGS = 0x00000002 (matches this client's bit; does not collide with the server's existing CAP_ZONE = 0x01), and
2. Honoring the flag emits CACHE_RESET so the client's dict mirror stays in lockstep.

The cited QwpEgressQueryFlagsResetTest / QwpEgressCacheResetWireTest should cover exactly this — verify they land and pass.

---

Minor

m1 — QUERY_FLAG_* constants are byte; sign-extension trap for any future bit-7 flag.
QwpEgressMsgKind.java:69 declares QUERY_FLAG_RESET_DICT = 0x01 as a byte. In resolveQueryFlags (QwpQueryClient.java:1775-1777) the ternary mixes that byte with 0L, so it's widened to long by sign extension. Fine today (0x01 → 1L). But the first future flag with bit 7 set — e.g. (byte)0x80 — widens to 0xFFFF_FFFF_FFFF_FF80L; putVarint's signed while (value > 0x7F) guard (NativeBufferWriter.java:310) then emits a single truncated 0x80 byte (continuation bit set, no follow-on) — a malformed varint. Cheap hardening: declare the QUERY_FLAG_* constants as int, or mask in resolveQueryFlags (& 0xFFL). Not a current bug.

m2 — No test for the reset flag together with bind parameters.
QwpQueryClientQueryFlagsTest only exercises the no-bind overloads, and its queryFlagsTrailer parser explicitly assumes zero binds. The client emits the trailer identically regardless of binds, so risk is low — but the highest-risk wire interaction (trailer after a non-empty bind section) is unpinned in this repo. Server-side decode of that case is covered by QwpEgressRequestDecoderTest in the tandem repo, which mitigates it. Consider one local case: execute(sql, binds, handler, true) with a bind, asserting the trailer lands after the bind payload.

m3 — TestWebSocketServer footgun: capabilities written without a matching zone_id trailer.
buildServerInfoFrame now writes the capabilities int verbatim but never emits a zone_id trailer. Since the client decoder reads a mandatory zone_id whenever the CAP_ZONE (0x01) bit is set, a future test that calls setCapabilities with bit 0 would make connect() fail with a decode exception. The new setCapabilities Javadoc warns about this and no current test sets CAP_ZONE, so it's contained — but a trap for the next caller.

m4 — Silent flag-drop gives the caller no direct signal (intended).
When the server doesn't advertise CAP_QUERY_FLAGS, resetSymbolDict=true is silently dropped and the caller gets a bloated Categorical with no indication. This is documented as intended (throwing would break rolling upgrades / old servers, so the choice is right). Callers needing certainty can inspect getServerInfo().getCapabilities() & CAP_QUERY_FLAGS — worth a one-line mention in the execute(..., boolean) Javadoc so users know the check exists.

---

PR metadata

Clean. Valid Conventional Commits title, correct qwp scope, end-user-impact-first description, enhancement label fits. No Fixes #NNN is correct (feature with a tandem PR, not an issue fix).

---

Dropped during verification (false positives)

• Stale queryFlags leaks across queries via the reused holder — overwritten on every submitQuery (QwpEgressIoThread.java:396).
• Failover replays the flag against a stale capability — re-resolved per attempt after reconnectViaTracker republishes serverInfo.
• New boolean overload changes resolution of existing callers — distinct parameter types; all callsites (incl. QueryImpl.java:177) bind unambiguously to their original overloads.
• Trailer could be misread as bind data — count()/bufferLen() move together; count>0 && bufferLen==0 is unreachable.
• Capture race / flaky test — server sets captured (AtomicReference) before sending EXEC_DONE; server.close() joins the read threads.

---

Findings: 10 draft → 5 verified (1 Moderate, 4 Minor), 5 dropped. 0 out-of-diff caller bugs (all callsites + submitQuery's sole caller walked and confirmed safe). Only tradeoff is the documented, opt-in loss of cross-query dict reuse when the flag is set; baseline frames stay byte-identical on the wire.

_{🤖 Review generated with Claude Code (Opus 4.8, 1M context)}

[bluestreak01]

Code review (level 3 — full pass)

Change-surface map + 11-reviewer fan-out (incl. fresh-context adversarial & cross-context caller walk) with per-finding source verification. Wire layout and constants were cross-checked byte-for-byte against the tandem server branch, and the full core suite was run (1263 tests, 0 failures, JDK 25).

Committed-binary gate: PASS — 5 text files, no binaries.

Verdict: Approve — no blocking issues

@kafka1991 one item needs your attention before/right after merge: please address M2 below (the pooled facade cannot reach the new flag) — either plumb resetSymbolDict through Query/QueryImpl in a fast-follow, or explicitly document the feature as QwpQueryClient-only. Everything else is advisory.

---

Moderate

M1 — No client-side wire test for the flag combined with bind parameters (in-diff test gap)
QwpEgressIoThread.sendQueryRequest:716-724 appends the trailer after the bind payload — the bind section is the only frame-layout interaction the feature has, and it is exactly the case the new test excludes (queryFlagsTrailer hard-asserts bindCount == 0). The encode order was verified correct against the tandem branch's QwpEgressRequestDecoder.decodeQueryRequest (trailer read iff p < limit after the bind loop), so this is a regression-detection gap, not a live bug — but this repo has no wire capture of any bound QUERY_REQUEST, and if bindPayloadLen bookkeeping ever drifted, the server would misread the trailer as bind bytes. Cheap fix reusing the existing byte-diff technique: capture execute(sql, b -> b.setLong(0, 42L), handler, false) vs ..., true), zero the request id, assert on.length == off.length + 1, prefix byte-identical, last byte QUERY_FLAG_RESET_DICT.

M2 — The pooled facade cannot reach the feature (out-of-diff; QueryImpl.runOn — core/src/main/java/io/questdb/client/impl/QueryImpl.java:348) ⬅ please address
QueryImpl.runOn calls the 3-arg execute(sqlBuffer, wireBinds, wrappingHandler) → flags always 0, and Query exposes no reset knob. Users of the pooled facade — which this repo's own docs (#61) promote as the first-class API — cannot set the flag at all; only raw QwpQueryClient users can. Needs a product decision: plumb the flag through Query/QueryImpl (follow-up PR is fine) or explicitly document the feature as QwpQueryClient-only.

---

Minor

m1 — QUERY_FLAG_RESET_DICT is byte; sign-extension trap for any future bit-7 flag (QwpEgressMsgKind.java:69)
The ternary in resolveQueryFlags (QwpQueryClient.java:1804-1807) widens the byte to long by sign extension — fine for 0x01, but a future (byte) 0x80 flag becomes negative, and NativeBufferWriter.putVarint:307-316 (while (value > 0x7F) — signed) would emit a single dangling 0x80 continuation byte: a malformed varint. The server-side counterpart declares this constant as int. Declare flag constants int/long to match, or mask in resolveQueryFlags.

m2 — Stale invariant comment (QwpEgressIoThread.java:217-222)
The comment attributes CACHE_RESET solely to the soft cap and claims it "never arrives between the RESULT_BATCH … of a query and the user callback, only after it." With this feature, the server (verified: QwpEgressUpgradeProcessor:1055 + streamResults on the tandem branch) emits CACHE_RESET at the head of a flagged query's stream. Handling is order-independent so behavior stays correct, but update the comment.

m3 — PR body / javadoc / label fixes

• Stale test name: the tandem class is QwpEgressQueryFlagsResetWireTest (verified via gh pr diff 7321 --name-only), not QwpEgressQueryFlagsResetTest — wrong in the PR body and in QwpQueryClientQueryFlagsTest.java:60.
• Two bare #7321 references ("server PR #7321", "the tandem PR #7321") linkify to this repo's #7321 — qualify as questdb/questdb#7321.
• The repo has a tandem label; apply it.
• Nits: "fastest" / "maximally efficient" are superlatives per convention; the tandem PR's title ("add per-query SYMBOL dict reset flag") is more precise than "scope the SYMBOL dictionary to a single query", which can read as an unconditional default change.

m4 — Test regressions manifest as hangs, not failures (QwpQueryClientQueryFlagsTest.java)
No @Test(timeout=...); takeEvent() blocks forever; CapturingQueryServer.onBinaryMessage drops non-QUERY_REQUEST frames without replying and swallows the IOException from the EXEC_DONE send. A regression that changes the request's msg_kind or breaks the reply leaves execute() blocked until the CI kill instead of failing with an assertion. Add a timeout (or reply with QUERY_ERROR to unrecognized frames).

m5 — Fixture doc style (TestWebSocketServer.java:290-292)
setCapabilities carries a // comment; every other public setter in the file uses javadoc. Convert for consistency. Keep the CAP_ZONE-unsupported warning — a future test setting bit 0 would fail connect() since the builder emits no zone_id trailer.

m6 — Additional unpinned behaviors (test suggestions)

• Per-attempt flag re-resolution in the failover loop is deliberate design nothing pins — a refactor hoisting resolveQueryFlags out of the executeImpl loop would regress silently.
• Flag-off frame byte-identity between a CAP_QUERY_FLAGS-advertising and non-advertising server is not asserted.
• queryFlagsTrailer never asserts full frame consumption after the trailer, so the trailer-width pin lives only in testResetTrailerIsTheOnlyDifferenceFromBaseline.

m7 — Binary-compat nit
The 6-arg submitQuery was removed from public class QwpEgressIoThread — internal I/O class, all in-repo callers updated, but technically a binary-incompatible change for anyone linked against it.

---

Downgraded (dismissed after source verification)

1. "Server must emit CACHE_RESET or the client dict desyncs" — verified satisfied on the tandem branch (QwpEgressUpgradeProcessor:1055 applies the reset between queries and emits the frame; constants match, no CAP_ZONE collision); a hypothetical desync fails safe via the deltaStart != connDictSize guard (QwpResultBatchDecoder:749).
2. Flag under failover causes dict desync — every reconnect builds a fresh QwpEgressIoThread whose decoder is a field initializer (QwpEgressIoThread:56, spawn at QwpQueryClient:1851), so connDictSize starts at 0 per connection.
3. Overload ambiguity for execute(sql, null, handler) — the new overload requires primitive boolean at position 3; QwpColumnBatchHandler has 3 abstract methods (not lambda-targetable). Full suite compiles and passes.
4. Stale queryFlags carryover on the reused pendingRequest — submitQuery assigns all 7 fields unconditionally before every put (QwpEgressIoThread:390-397); ArrayBlockingQueue hand-off gives the happens-before edge.
5. resolveQueryFlags sees serverInfo == null mid-close and misbehaves — close() never nulls serverInfo; nulling happens only on the executing thread in cleanupFailedConnect:1408, and the null branch safe-drops to 0.
6. Test capture race / flake — the server allocates a fresh byte[] per frame, the CAS commits before the EXEC_DONE send, and execute() returns only after EXEC_DONE is consumed.
7. TestWebSocketServer change breaks the 32 consumer tests — default capabilities=0 reproduces the old hardcoded putInt(0) byte-for-byte; no existing test calls setCapabilities; full suite green.
8. Reinvented test helpers — no shared readVarint/EXEC_DONE builder/NOOP_HANDLER helpers exist; per-file copies are the established convention; TestPorts is for dead-endpoint ports, not live fixtures.
9. io/serverInfo tear under concurrent connect()+execute() — requires documented misuse that already clobbers ioThread/webSocketClient far more destructively; pre-existing pattern, and a stray trailer is ignored by the server decoder anyway. (Optional hardening: snapshot serverInfo alongside io at executeOnce entry.)

---

Summary

• Flag-off wire is byte-identical (pinned by test), capability gating is re-evaluated per failover attempt on the correct thread, the cross-thread hand-off has a proper happens-before edge, and the wire layout/constants match the tandem server branch byte-for-byte. Zero allocations, no per-row cost, Java-8-floor clean (validated on JDK 25; CI's JDK 8 job remains the authoritative gate).
• Merge dependency: land together with feat(qwp): add per-query SYMBOL dict reset flag questdb#7321; this PR fails safe on its own (flag silently dropped without the capability), but the feature is inert until the server half merges.
• Verification tally: ~20 draft claims → 13 findings verified (2 Moderate, 11 Minor/nits), 9 dismissed as false positives.
• In-diff vs out-of-diff split: 11 in-diff, 2 out-of-diff (M2 at QueryImpl.java:348; m2 at QwpEgressIoThread.java:217-222). Cross-context pass walked every callsite of every changed symbol — all SAFE, corroborated by the full-suite run.

[mtopolnik]

[PR Coverage check]

😍 pass : 18 / 18 (100.00%)

file detail

	path	covered line	new line	coverage
🔵	io/questdb/client/cutlass/qwp/client/QwpQueryClient.java	15	15	100.00%
🔵	io/questdb/client/cutlass/qwp/client/QwpEgressIoThread.java	3	3	100.00%