docs(api,deployment): examples and BYOS notes for the positional wire - #555
Merged
Merged
Conversation
Both decisions in classify-paths.sh were `if printf … | grep -qE …; then A; else B; fi`. grep exits 0 on match, 1 on no match, and 2 on error — can't fork/exec, read error, bad pattern — and the else branch collapsed 1 and 2 into the same answer. `set -euo pipefail` does not help: `set -e` is suppressed for a command used as an `if` condition. Observed twice while gating this stack, in runs whose static checks run at -j 14. A different single case failed each time — `mixed-docs-go` answering docs=false, then `dep-bump-go` answering code=false — while every other case passed. That is the signature of a transient grep failure under load, not a pattern bug; the script and its test are unchanged from main and both pass standalone. The test caught it because it asserts expected values. The production path has no such check: CI's `changes` job gates the docs pipeline on this answer, so a docs=false produced by an errored grep skips the docs build and still reports success. Both greps now go through a `matches` helper that aborts with a diagnostic on any exit above 1. The test suite stubs grep onto PATH to prove the abort fires — that case fails against the previous script, which answers confidently instead. Closes #545. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
Additive metadata the upcoming native type layer needs, captured on the same refresh as the columns so they can never describe different servers: - Column gains DefaultExpression and Position, both scanned from the widened system.columns select. TableSchema.Columns was already ordered by position, so declaration order needed no new structure. - TableSchema gains DDL from system.tables.create_table_query. It is json:"-": the schema endpoint marshals TableSchema straight to the client and an external-engine table (S3, MySQL, Kafka) carries its credentials in that statement. A table listed in system.tables with no system.columns rows is skipped, never published column-less. - SchemaRegistry gains ServerVersion(), from a SELECT version() probe next to the existing SELECT timezone(). Both new queries fail the refresh on error, matching timezone() and system.columns: callers keep the prior cache and retry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
`tables.<table>.select.<role>` becomes `tables.<table>.<role>.select`. Field
names and semantics are unchanged; only the nesting moves. `select` and
`insert` are now distinct types (`SelectPermissions`/`InsertPermissions`), so a
field on the wrong side is a validation error instead of being accepted and
ignored.
There is no automatic conversion — convert the file by hand and run
`wavehouse validate` before restarting. A pre-v2 document is reported as one
clear finding pointing at the migration note rather than a confusing
strict-decode error.
Three fail-closed fixes the new shape made visible or possible:
- `ResolvedPermissions` now marks the side `Evaluate` did not resolve. An
unresolved side is zero, and a zero side reads as an empty allow list plus an
empty deny list — which every accessor would answer as "unrestricted". Each
one now denies instead, `HasRowFilter` included: it is the reachable gate in
front of `RowVisible`, so without it the guard behind it was dead code and
both call sites took the whole-bucket fast path.
- `evaluateInsert` resolved a check using an operator it does not honor
(`_neq`/`_gt`/`_lt`, or the ambiguous `_eq`+`_in`) to no clause at all,
authorizing the insert with the rule silently gone — where `evaluateSelect`
denies outright in the mirror situation.
- A `filter` or `check` entry naming no operator (`"tenant_id": {}`) survived a
strict decode, passed `Validate`, and matched no case in either resolver.
Reproduced before fixing: `Allowed=true`, `HasRowFilter()=false`, and
`RowVisible` true for another tenant's row — a declared row-level restriction
applying on neither the query path nor the live stream.
Note on scope: an earlier revision of this work also made
`POST /v1/ops/policy/validate` agree with file adoption. #541 deleted the
policy HTTP surface entirely, so that fix is gone with it — the legacy-layout
detector it relied on is still reached through file adoption's own pipeline.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
`POST /v1/ingest` used to sniff the body and treat the header as a hint: the first non-whitespace byte chose between a single object and an array, and an NDJSON body whose first line happened to start with `[` was re-framed as a JSON array — silently, as a whole-request reinterpretation rather than a per-record error. The header is now required and authoritative. A request declaring nothing, or something ingest does not read, is `415` before the body is parsed, with the supported types named in the body. The declared type chooses the format *family* — `application/json` versus the four NDJSON spellings — and within the JSON family the first non-whitespace byte still picks array versus single object. The bytes never choose the family, so an NDJSON body is read as NDJSON whatever its first byte and a bad line fails as a per-record error. Parameters are ignored (`application/json; charset=utf-8` is `application/json`), matching `mime.ParseMediaType`. The TS SDK already sent `application/json` on every ingest call, so `.insert()` is unaffected. A hand-rolled client that relied on sniffing must now declare the type. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
…encoder Observable behavior is unchanged. This is the seam work the native type layer lands against, plus the memory trade that comes with it. The handler now reads the whole (already `MaxBytesReader`-capped) body into a pooled `*bytes.Buffer` and runs the record readers over those bytes rather than the live connection, so the `413` surfaces at that read instead of mid-iteration and the `415` is decided from the header before a byte is read. The memory profile is NOT unchanged, and that is the deliberate part. Streaming meant peak resident bytes on the order of one record — NDJSON scanned line by line, the array path let `json.Decoder` compact after each element. Peak is now O(body) per in-flight request, and `bytes.Buffer` doubles, so peak allocation can exceed the cap before `MaxBytesReader` errors. `maxPooledBufferBytes` (1 MiB) caps what a request hands back to the pool, not its peak, and nothing bounds total in-flight bytes — the ceiling is concurrency × the 16 MiB data-plane cap, which has no operator knob, so the outer limit is the proxy's. Kept because the type layer needs the body addressable rather than consumed; a server-side bound is tracked in #544, and the reverse-proxy guide now says to size the container for concurrency rather than for one request. Three decision points became interfaces with default implementations that delegate to today's code unchanged: `RecordValidator` (schema validation + timestamp canonicalization — the two calls stay where they are, with the check-clause block between them, since merging them would move checks onto canonicalized values), `InsertChecker` (the `_eq` and `_in` comparisons), and `stream.RowEvaluator` (row visibility, reached by both the live fan-out and replay through the one shared admission step). `ingest.EncodeCompactRow` lands here unused — the positional encoder the wire change ahead of it will call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
NATS envelope v2 (BREAKING). `EventMessage`'s `data` object is replaced by
`format` ("JSONCompactEachRow"), `columns` (the table's declaration order) and
`row` (one compact line — a positional JSON array). A batch carries the column
names once instead of repeating every key on every row, and a reader can tell a
schema change mid-stream from a reordering.
In-flight messages published by an older version are not readable by the new
worker — an envelope whose `format` is absent or unknown, or whose `columns`
and `row` cannot be paired, carries no way to say which value belongs to which
column. DRAIN THE INGEST QUEUE BEFORE DEPLOYING. Such an envelope is parked on
the DLQ, or acked-and-dropped with an ERROR log and a
`wavehouse_ingest_poison_dropped_total{table,reason}` increment where the DLQ
is off — never left unacked to redeliver forever, and never inserted.
The worker groups a batch by column list, so a schema change mid-stream splits
the INSERT rather than corrupting it, and writes
`INSERT INTO {table} (cols) FORMAT JSONCompactEachRow`. Row cells are copied as
their original bytes rather than re-encoded at each hop, so a 64-bit id past
2^53 keeps every digit end to end.
SSE (BREAKING for raw consumers). A data frame's `data` object is replaced by
`row`, and each connection is sent an `event: schema` frame naming its
projected columns before its first row and again on drift. The TS SDK consumes
it and still yields row objects, so `.stream()`, `.liveQuery()` and
`StreamEvent.data` are unchanged; a raw `EventSource` consumer must now zip
rows itself.
Upgrade the SDK and the server together — they share this wire protocol, and a
skewed pair delivers no usable rows and raises no error.
A row is never queued without its announcement: if the queue is full when the
announcement is offered, the row is dropped too and the signature is not
recorded, so the next event announces again. A slow consumer loses a row rather
than receiving one it would zip against a stale column list. Both drops are
counted under their own frame kinds — the withheld row explicitly, since it is
never offered to Send.
Replay and the live fan-out track drift in separate state and do not reconcile
it; the residual same-length case is #543.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
Naming columns in the INSERT made a MATERIALIZED or ALIAS column fatal.
Verified against ClickHouse 26.7.3: a MATERIALIZED column in an INSERT column
list is `Cannot insert column …, because it is MATERIALIZED column` (code 44,
and `insert_allow_materialized_columns` defaults to 0); an ALIAS column is
`No such column …` (code 16).
Schema discovery reads every row of `system.columns` with no `default_kind`
filter, so both landed in the envelope and then in the statement: a table
carrying either could ingest under the previous column-less `FORMAT
JSONEachRow` and could not ingest at all under the new one — every row to the
DLQ, or redelivered forever where the DLQ is off. No fixture in the suite
declared such a column, which is why every gate was green while this was
broken; tests/integration now creates one and drives the real path end to end.
Discovery keeps `default_kind` alongside each column and exposes `IsInsertable`
/ `InsertableColumns` / `InsertableColumnNames`; the envelope, the compact
encoder and the SSE connect-time announcement use that subset, while
`GET /v1/ops/schema` still reports the whole table. EPHEMERAL stays insertable
— it is insert-only by construction, never stored, confirmed on the same server
rather than assumed. The subsets are memoized per table per refresh, since the
ingest path would otherwise rebuild them once per record.
BREAKING: a record that SUPPLIES a value for a MATERIALIZED/ALIAS column is now
rejected (400 … cannot be inserted) where the positional encoder previously
accepted it and silently dropped the value.
BREAKING: a policy `check` naming a column the table does not have, or one it
computes, is now refused per-record with a 403 naming the column and the table.
Such a clause could never be enforced — the published row carries one slot per
insertable column, so an auto-injected value for anything else was dropped on
the way out and the record inserted WITHOUT it, answering 200 {"ok":true}.
Policy validation cannot catch this (it never sees the ClickHouse schema), so
audit your check blocks against their tables before upgrading; `wavehouse
validate` will not tell you.
Also here: biome.json declared a `$schema` version that did not match the
pinned biome — a one-line drive-by, unrelated to the above.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
📝 SummarySummary by CodeRabbit
WalkthroughThe changes align schema, ingest, SSE, SDK, deployment, changelog, and DLQ documentation with current behavior. DLQ integration tests now serialize typed ChangesIngest contract documentation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🔵 Low · up to The schema documentation improves ingest and discovery guidance, but its metadata example still understates computed-column metadata. SDK consumers could misinterpret discovered schema fields until this wording is corrected. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (5 skipped: 5 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
6 tasks
Member
Author
|
@coderabbitai review Auto-review is off for PRs whose base isn't the default branch, and this one sits in a stack (base Note for context: this PR's diff against its base is one layer of a seven-PR stack replacing #540. Reviewing it in isolation is the intent — the layers below it are already in its history. |
|
|
This was referenced Sep 2, 2026
`database` is a live getter so a ClickHouse reconfigure is honored on the next refresh. Reading it once per query rather than once per refresh let a reconfigure land between the system.columns and system.tables scans, attaching DDL from the new database to same-named schemas discovered from the old one — silently, and it would survive into GET /v1/ops/schema. Refresh now snapshots the name before the first query and threads it to both. Both refresh-failure tests asserted emptiness after a single failed refresh, which is satisfied by the regression they exist to prevent — a failure that wipes an already-published registry. Both now seed a successful refresh first. TestRefresh_DatabaseSnapshottedForWholeRefresh flips the getter between the two scans and fails without the snapshot. CORRECTS TWO CLAIMS IN THIS BRANCH'S EARLIER COMMIT (e10eb04), whose message is already pushed and cannot be amended under the no-force-push rule: 1. "an external-engine table carries its credentials in that statement" is FALSE on the ClickHouse this repo ships. Verified on 26.7.3.19: MySQL, PostgreSQL, S3, Kafka SASL, MongoDB URIs and S3 presigned signatures all render the secret as `[HIDDEN]`. What create_table_query does leak, unconditionally, is topology — endpoint, bucket or host, database, username, S3 access key id. `json:"-"` is still right, for that stronger reason. The password is exposed only on a pre-masking server or one with the server-level display_secrets_in_show_and_select enabled. 2. "captured on the same refresh so they can never describe different servers" overstated the guarantee. chconn.Manager resolves the connection per call, so a reload changing clickhouse.addr mid-refresh can pair a version from one server with schemas from another. It is a publication guarantee, not a same-server one. The read side straddles too: ServerVersion() and Get() take separate RLocks. The rationale is corrected at every site it was replicated — discovery.go, AGENTS.md, architecture.md, api.md, the CHANGELOG — and the test fixture, which had been asserting a literal secret no real server emits, now uses masked DDL and asserts on the bucket and access key id instead. Adds tests/integration/discovery_metadata_test.go: position is 1-based and contiguous against live ClickHouse, DefaultExpression and HasDefault behave as documented for a MATERIALIZED column, DDL is captured, ServerVersion is set. Raised by CodeRabbit on #550 and by the pre-push reviewer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
The unsupported-media-type test checked the status and two of the five accepted types. It now asserts the JSON error envelope and its security headers via testutil.AssertJSONErrorResponse — which this branch was missing, though the same assertion exists further up the stack, so the split dropped it onto the wrong layer — and iterates supportedContentTypes. On what that loop pins: it cannot catch an alias being dropped from supportedContentTypes, because the message is built from the same slice and the loop would simply check one fewer. Verified. What it does catch is the message diverging from the list, so a client is never told to use a type the server accepts but never names; verified by truncating the Join and watching it fail. Also rewords the documented 415 cause in three places: "or one ingest doesn't read" was not a condition a reader could act on. Now "or an unsupported one". Raised by CodeRabbit on #552. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
The pre-v2 layout detector treated `tables.<table>.select` as an operation map whenever its value's keys were not permission fields, so a valid v2 grant for a role literally named `select` was rejected before the strict decode. Raised by CodeRabbit on #551. The first fix bailed out whenever every key was an operation name. That was wrong, and wrong in the dangerous direction — it made this document adopt: {"tables":{"clicks":{"select":{"select":{"allow_columns":["a"]}, "insert":{"allow_columns":["b"]}}}}} Pre-v2 that grants two read-only roles. Read as v2 it grants ONE role read and write: role `select` gained an INSERT the file never contained, and role `insert` silently lost its SELECT. No error, no warning. Caught by the pre-push reviewer; reproduced before this fix. The discriminator is narrower. When the inner key set is exactly the outer operation (`select.select`), the v1 and v2 readings describe identical access, so accepting the v2 decode is free. When it names the OTHER operation (`select.insert`) the readings diverge, and that is precisely the escalating case — so it is refused, with its own message asking for a rename rather than the migration message, which would tell an operator to convert a file that may already be v2. TestReportLegacyPolicyLayout_RoleNamedAfterAnOperation pins all three outcomes and asserts the divergent case does not get the migration message. The previous version of that test asserted the escalating shape as safe. Also corrects `settings-directory.mdx`, which said a leftover `"select": {}` block always fails the undeclared-role check first — that holds only when `roles.json` does not declare a role named `select` — and qualifies the "reported as one clear finding" claims in the CHANGELOG and access-control.mdx, which are not true of the ambiguous shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ
#554 landed as squash 4f05b18. This branch had merged #554 at 141a54f, BEFORE the eight review commits that PR accumulated, so main carried work the branch did not have. That makes `-s ours` wrong here, and the interlock is what caught it: its second condition is that the squash's content already be present in the branch, and it was not. `-s ours` discards main's side, so it would have silently reverted the whole #554 review round the moment #555 merged -- the deleted SSE unpairable metric, the newEventView format check, the poison counter rename and both of its ack-ordering fixes, the docs SDK pin, the dependabot ignore, and the zero-column pairRow guard. A normal merge instead, resolved per hunk. Resolution rule: main is authoritative for anything #554 owns, and the branch is authoritative for what its own commits authored (b09dbbe, d686fa5, db3d8e4, 8f017f4, 4a33fd1). Establishing which was which mattered -- diffing the branch tip against 141a54f conflates #555's work with #554 refinements the branch never received, so authorship came from `git log -S` per contested string rather than from the diff direction. Two things that would not have surfaced on their own. metrics.go auto-merged CLEANLY into a stale state, keeping the unpairable counter and RowUnpairable that #554 deleted -- orphaned dead code, no conflict marker, compiles and passes tests; the branch's own tip already matched main there, so the staleness came from its earlier merge of 141a54f. And taking main's CHANGELOG wholesale dropped the computed-columns bullet this PR exists to add, because a whole-file checkout takes the file, not the hunk; re-applied from b09dbbe. Verified after resolving: every #554 review artifact present, both removals actually gone, #555's computed-column exclusion intact, and the only files differing from main among #554's 50 are the six carrying #555's own authored content. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both from the reviewer, both introduced by my merge resolution. AGENTS.md still named wavehouse_ingest_poison_dropped_total. It was the only surviving instance in the repo, and I missed it because my stale-claim sweep grepped internal/, docs/src and CHANGELOG.md and never AGENTS.md -- the one file whose whole job is to be the invariant index a future reader trusts. Two more errors in the same sentence, fixed in the same pass: it attached the counter to the dropped case alone, though rejectPoison counts both dispositions and the disposition label is the entire point of the rename; and it listed two of the three unreadable cases, omitting malformed JSON. The computed-columns CHANGELOG bullet I re-applied after the whole-file checkout dropped it turned out to duplicate the entry main already carries -- #554 had absorbed this branch's work, so main's version is the fuller and corrected one. The two disagreed on the ClickHouse version: mine said 26.7.3, main's 26.6.3, and 26.6.3.62 is what the compose files, CI and the integration setup actually pin. Deleted mine, but folded in the one claim only it carried and which appears nowhere in main's changelog: that a record SUPPLYING a value for a computed column is now rejected 400 rather than accepted and silently dropped. Marked that entry BREAKING, which it was not. The other 26.7.3 in this file is pre-existing on main, in the schema-discovery entry, and is left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Docs reviewer, on the merge. It found the same two MUSTs the code reviewer did (AGENTS.md's revived metric name, the doubled CHANGELOG entry), both already fixed in 5269726; these are its refinements and its remaining findings. AGENTS.md said "unknown format" where every other site says "unknown OR ABSENT" -- a pre-v2 envelope carries no format field at all, which is the case the sentence exists to describe. And the CHANGELOG entry I folded into mentioned only default_kind, while /v1/ops/schema also reports default_expression and position (discovery.go:29-36, and the system.columns SELECT at :252); the deleted duplicate had that claim and it appears nowhere else, so it came along. api.md announced ["page","button","received_timestamp"] for the stream example, silently dropping score, while the frame 230 lines later drops received_timestamp instead and explains itself as a role restricted to page/button/score. Two different projections of one table, only one of them accounted for -- and a reader holding the four-column DDL sees an announcement that is not even a declaration-order prefix. That one was mine: I took the branch's side for those hunks to keep its received_timestamp collision note, without noticing the projection it implied. The example is now the full insertable list, so the only projected frame in the page is the one that says whose projection it is. sdk/queries.md pinned the canonical four-column clicks in its .schema() output while its own insert examples sent {page, button} and {"page":"/a"} -- both 400, since score and button are non-nullable with no default. Same class the branch had already corrected in api.md; the insertNDJSON paragraph was edited here without its sample coming along. Not fixed here: development.md's dedupe walkthrough posts a record with an unknown column and annotates two responses the server cannot send. Pre-existing, outside this branch's delta, and it needs the page's shared clicks DDL changed, which every other example there reads against. Filed as #575 rather than widened into this PR. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
@coderabbitai review This branch is now rebuilt on top of main after #554 merged (squash The situation: this branch had merged #554 at Two traps I hit, in case they suggest others:
Two local reviewers already ran and independently converged on the same two findings ( Specifically worth checking: whether any #554 artifact was lost or any stale claim reintroduced anywhere I did not look; whether Note this PR is still a draft — un-drafting is a human-only action here, so it is waiting on Eric for that and for the required approval. |
|
📚 Docs preview is live → https://56ed403b-wavehouse-docs.wave-rf.workers.dev
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/src/content/docs/sdk/queries.md (1)
76-77: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDocument computed schema metadata.
default_kindanddefault_expressionare not limited to ordinaryDEFAULTcolumns. The discovery contract also reports computed kinds such asMATERIALIZED; the integration test verifiesMATERIALIZEDandEPHEMERALmetadata. Clarify this sentence so SDK consumers do not ignore computed-column metadata.Suggested wording
-// `default_kind`/`default_expression` appear only when the column declares a default. +// `default_kind`/`default_expression` describe ClickHouse default-expression +// kinds, including computed kinds such as `MATERIALIZED` and `ALIAS`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: e18c8b1f-7b0e-45ad-ab04-2c04e40834fe
📒 Files selected for processing (5)
AGENTS.mdCHANGELOG.mddocs/src/content/docs/api.mddocs/src/content/docs/deployment.mddocs/src/content/docs/sdk/queries.md
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Docs build
- GitHub Check: Integration tests
- GitHub Check: E2E tests
- GitHub Check: Coverage
- GitHub Check: Lint
🧰 Additional context used
📓 Path-based instructions (1)
Never hard-wrap prose.
📄 CodeRabbit inference engine (AGENTS.md)
Files:
docs/src/content/docs/deployment.mdAGENTS.mddocs/src/content/docs/api.mdCHANGELOG.mddocs/src/content/docs/sdk/queries.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: EricAndrechek
URL: https://github.com/Wave-RF/WaveHouse/pull/555
Timestamp: 2026-09-05T02:12:22.899Z
Learning: For WaveHouse SSE delivery, `internal/stream/hub.go` uses separate schema-drift state for live `Hub.Broadcast` delivery and `Hub.ReplayProjector` gap-fill replay. If a connection gap-fills across a column change, subsequent live rows can arrive without a fresh `event: schema` frame until the next drift or reconnect. Documentation must qualify any claim that schema frames are sent before every changed column list.
Learnt from: EricAndrechek
Repo: Wave-RF/WaveHouse
Timestamp: 2026-09-05T02:11:37.031Z
Learning: In the Go SSE implementation in `internal/stream/hub.go`, `Hub.ReplayProjector` keeps gap-fill schema-drift state in closure-local `lastSig`, while `Hub.SubscribeSchemaFrame` records the current schema signature on the connection's `stream.Subscriber`. These states are not reconciled. If a connected client gap-fills across a table column change, subsequent live rows can arrive without a fresh `event: schema` frame until the next drift or reconnect. An arity check detects added or removed columns but cannot detect same-length changes such as `RENAME COLUMN` or a paired drop/add. This known limitation is tracked by GitHub issue `#543`.
Learnt from: EricAndrechek
URL: https://github.com/Wave-RF/WaveHouse/pull/555
Timestamp: 2026-09-05T02:59:44.363Z
Learning: In WaveHouse documentation, schema-frame claims were historically authored independently from the `internal/stream/hub.go` `KNOWN LIMITATION` comment. A correction in one document does not imply that equivalent claims elsewhere are corrected. When reviewing changes to the SSE schema-frame contract, search `AGENTS.md`, maintained documentation, and READMEs for independently worded claims, then verify each against the live `Hub.Broadcast` and gap-fill `Hub.ReplayProjector` behavior.
🪛 LanguageTool
CHANGELOG.md
[typographical] ~23-~23: Consider using an em dash in dialogues and enumerations.
Context: - **The docs site now consumes the *publi...
(DASH_RULE)
[style] ~23-~23: Since ownership is already implied, this phrasing may be redundant.
Context: ...gainst a separately-deployed backend on its own release cadence, but took its SDK from ...
(PRP_OWN)
[style] ~23-~23: ‘for want of’ might be wordy. Consider a shorter alternative.
Context: ...ing "live" while every frame is dropped for want of a schema announcement, with no error ...
(EN_WORDINESS_PREMIUM_FOR_WANT_OF)
[style] ~23-~23: Since ownership is already implied, this phrasing may be redundant.
Context: ...ately keeps workspace:*. Depending on our own package from the registry also made `mi...
(PRP_OWN)
[style] ~24-~24: Since ownership is already implied, this phrasing may be redundant.
Context: ...fferent operation, or both — so it gets its own error asking you to rename the role rat...
(PRP_OWN)
[grammar] ~30-~30: Use a hyphen to join words.
Context: ...ngest → NATS → the worker's INSERT end to end. BREAKING: a record that *suppli...
(QB_NEW_EN_HYPHEN)
[style] ~58-~58: Consider an alternative for the overused word “exactly”.
Context: ...JSON, an unknown row format (which is exactly what a pre-v2 message looks like), or c...
(EXACTLY_PRECISELY)
🔇 Additional comments (5)
docs/src/content/docs/deployment.md (1)
316-318: LGTM!Also applies to: 338-338, 340-340, 351-351, 354-354
docs/src/content/docs/sdk/queries.md (1)
33-38: LGTM!Also applies to: 49-53
docs/src/content/docs/api.md (1)
238-238: LGTM!Also applies to: 268-268, 334-334, 343-345, 395-403, 600-610, 807-808, 821-821, 825-835, 839-839, 858-858
AGENTS.md (1)
46-46: LGTM!Also applies to: 56-56, 57-57
CHANGELOG.md (1)
30-30: LGTM!Also applies to: 58-58
EricAndrechek
marked this pull request as ready for review
September 9, 2026 00:40
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Wave-RF/WaveHouse"
expected_head="1ada6498"
echo '=== Pull request metadata ==='
gh pr view 555 --repo "$repo" \
--json number,url,state,isDraft,baseRefName,baseRefOid,headRefName,headRefOid,commits,files \
--jq '{
number, url, state, isDraft, baseRefName, baseRefOid, headRefName, headRefOid,
commits: [.commits[] | {oid, messageHeadline}],
files: [.files[] | {path, additions, deletions}]
}'
echo
echo '=== Local refs and ancestry ==='
git rev-parse HEAD
git rev-parse main
git merge-base main HEAD
git merge-base --is-ancestor main HEAD && echo 'main_is_ancestor_of_HEAD=true' || echo 'main_is_ancestor_of_HEAD=false'
git show -s --format='HEAD %H%nparents %P%nsubject %s%n%nbody%n%b' HEAD
git show -s --format='main %H%nsubject %s' main
echo
echo '=== Merge and fix commits named by the author ==='
for commit in 4f05b184 141a54ff b09dbbe6 d686fa51 db3d8e40 8f017f47 4a33fd13 52697261 1ada6498; do
if git rev-parse --verify -q "${commit}^{commit}" >/dev/null; then
git show -s --format='%H%nparents %P%nsubject %s%n' "$commit"
else
echo "missing locally: $commit"
fi
done
echo
echo '=== Current diff from main ==='
git diff --name-status main...HEAD
git diff --stat main...HEAD
git diff --find-renames --find-copies --unified=35 main...HEAD -- \
AGENTS.md CHANGELOG.md \
docs/src/content/docs/api.md \
docs/src/content/docs/deployment.md \
docs/src/content/docs/sdk/queries.md \
docs/src/content/docs/sdk/streaming.md \
docs/src/content/docs/architecture.md \
clients/ts/src/types.ts \
tests/integration/dlq_test.go \
internal/stream/metrics.go
echo
echo '=== `#554` artifact checks: implementation ==='
rg -n -i -C 8 \
'(RowUnpairable|unpairable|wavehouse_ingest_poison_(dropped_)?total|newEventView|pairRow|input_format_null_as_default|IsInsertable|MATERIALIZED|ALIAS|groupByColumns|ack)' \
--glob '*.go' internal tests
echo
echo '=== Locate and map targeted Go files ==='
fd -t f -i '^(metrics|hub|.*ingest.*|.*event.*)\.go$' internal tests | sort
for f in $(fd -t f -i '^(metrics|hub|.*ingest.*|.*event.*)\.go$' internal tests | sort); do
ast-grep outline "$f" --items all || true
done
echo
echo '=== Documentation contract checks ==='
rg -n -i -C 4 \
'(input_format_null_as_default|explicit.{0,30}null|non-nullable.{0,80}default|nullable\(.*\).{0,80}default|omitted.{0,80}(default|NULL)|MATERIALIZED|ALIAS|EPHEMERAL|computed column|insertable|schema frame|schema event|gap[- ]?fill|poison|DLQ|groupByColumns|per-table batching)' \
AGENTS.md CHANGELOG.md docs/src/content/docs clients/ts/src/types.ts
echo
echo '=== Full API documentation sections relevant to ingest and SSE ==='
sed -n '210,290p' docs/src/content/docs/api.md
sed -n '430,490p' docs/src/content/docs/api.md
sed -n '580,650p' docs/src/content/docs/api.md
sed -n '790,855p' docs/src/content/docs/api.md
echo
echo '=== Changelog computed-column and ClickHouse-version entries ==='
rg -n -i -C 8 '(computed|MATERIALIZED|ALIAS|26\.6\.3\.62|26\.7\.3|positional|insert column)' CHANGELOG.mdLength of output: 202427 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Refs and merge topology ==='
printf 'HEAD='; git rev-parse --short=12 HEAD
printf 'main='; git rev-parse --short=12 main
printf 'merge-base='; git merge-base --short main HEAD 2>/dev/null || git merge-base main HEAD | cut -c1-12
git log --oneline --decorate -12 HEAD
git show -s --format='commit=%H%nparents=%P%nsubject=%s' HEAD
git merge-base --is-ancestor main HEAD && echo 'main is an ancestor of HEAD'
echo
echo '=== Files changed since main ==='
git diff --name-status main...HEAD
git diff --stat main...HEAD
echo
echo '=== Merge-resolution commits and their file changes ==='
for c in 52697261 1ada6498; do
echo "--- $c ---"
git show -s --format='%H%nparents=%P%nsubject=%s' "$c"
git diff-tree --no-commit-id --name-status -r "$c"
done
echo
echo '=== Current diff from main: relevant files ==='
git diff --unified=12 main...HEAD -- \
AGENTS.md CHANGELOG.md docs/src/content/docs/api.md \
docs/src/content/docs/deployment.md docs/src/content/docs/sdk/queries.md \
docs/src/content/docs/sdk/streaming.md clients/ts/src/types.ts \
tests/integration/dlq_test.go internal/stream/metrics.go \
.github/dependabot.yml
echo
echo '=== Exact `#554` artifact identifiers in current source ==='
rg -n -C 4 \
'RowUnpairable|unpairable|wavehouse_ingest_poison_dropped_total|wavehouse_ingest_poison_total|newEventView|pairRow' \
internal tests AGENTS.md || true
echo
echo '=== Metrics, event-view, and row-pairing definitions ==='
for f in internal/stream/metrics.go internal/ingest/worker.go internal/ingest/event.go; do
if [ -f "$f" ]; then
echo "--- $f ---"
ast-grep outline "$f" --items all
rg -n -C 10 '(poison|unpairable|newEventView|pairRow|format)' "$f" || true
fi
done
echo
echo '=== Computed-column implementation ==='
rg -n -C 10 \
'(IsInsertable|InsertableColumns|MATERIALIZED|ALIAS|default_kind|INSERT INTO|groupByColumns)' \
--glob '*.go' internal
echo
echo '=== API documentation: validation, batch ingest, wire format, SSE ==='
rg -n -C 6 \
'(Validation|explicit `null`|input_format_null_as_default|MATERIALIZED|ALIAS|Batch|NDJSON|Client-Facing Format|event: schema|gap-fill|columns)' \
docs/src/content/docs/api.md
echo
echo '=== Changelog: positional/computed-column entry only ==='
rg -n -C 5 \
'(computed columns|MATERIALIZED|ALIAS|insertable column|positional encoder|26\.6\.3\.62|26\.7\.3)' \
CHANGELOG.md | head -n 180
echo
echo '=== Dependabot exception for eventsource-parser ==='
rg -n -C 5 'eventsource-parser|ignore' .github/dependabot.yml || trueLength of output: 202319 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Topology and current PR identity ==='
printf 'HEAD '; git show -s --format='%H %s' HEAD
printf 'main '; git show -s --format='%H %s' main
printf 'merge-base '; git merge-base main HEAD
git merge-base --is-ancestor main HEAD && echo 'RESULT main is an ancestor of HEAD'
git log --oneline --first-parent -8 HEAD
echo
echo '=== Paths that differ from main ==='
git diff --name-status main...HEAD
echo
echo '=== Main-versus-head changes in `#554-owned` implementation paths ==='
git diff --name-status main...HEAD -- \
internal/stream/metrics.go \
internal/stream/hub.go \
internal/ingest/worker.go \
.github/dependabot.yml \
docs/package.json \
pnpm-lock.yaml
git diff --check main...HEAD
echo
echo '=== Stale identifier absence in active source and docs ==='
for pattern in \
'RowUnpairable' \
'wavehouse_ingest_poison_dropped_total' \
'wavehouse_sse_unpairable' \
'merge -s ours'
do
echo "--- $pattern ---"
rg -n -F "$pattern" \
AGENTS.md CHANGELOG.md internal docs/src/content/docs clients/ts tests .github \
|| true
done
echo
echo '=== Current PR patch: compact documentation and test layer ==='
git diff --unified=8 main...HEAD -- \
AGENTS.md CHANGELOG.md clients/ts/src/types.ts \
docs/src/content/docs/api.md docs/src/content/docs/deployment.md \
docs/src/content/docs/sdk/queries.md docs/src/content/docs/sdk/streaming.md \
docs/src/content/docs/architecture.md tests/integration/dlq_test.go
echo
echo '=== Tests that pin computed-column ingest and schema-frame behavior ==='
rg -n -C 3 \
'(RejectsSuppliedComputedColumn|InsertableColumns|ExcludesComputedColumns|computedRegistry|materialized and cannot be inserted|alias and cannot be inserted)' \
internal/api/ingest_test.go internal/discovery/discovery_test.go internal/stream/hub_test.go
echo
echo '=== Documentation consistency: all high-risk claims excluding fenced examples ==='
python3 - <<'PY'
from pathlib import Path
import re
paths = [
Path("AGENTS.md"),
Path("docs/src/content/docs/api.md"),
Path("docs/src/content/docs/deployment.md"),
Path("docs/src/content/docs/sdk/queries.md"),
Path("docs/src/content/docs/sdk/streaming.md"),
Path("docs/src/content/docs/architecture.md"),
Path("clients/ts/src/types.ts"),
]
needles = re.compile(
r'(MATERIALIZED|ALIAS|EPHEMERAL|Nullable\(T\).*DEFAULT|'
r'input_format_null_as_default|schema frame|gap-fill|'
r'poison_total|poison.*dropped|insertable columns)',
re.I,
)
for path in paths:
print(f"--- {path} ---")
in_fence = False
for line_no, line in enumerate(path.read_text(errors="replace").splitlines(), 1):
if line.strip().startswith("```"):
in_fence = not in_fence
if not in_fence and needles.search(line):
print(f"{line_no}: {line.strip()}")
PYLength of output: 119617
The merge base is The The computed-column behavior is intact.
✏️ Learnings added
You are interacting with an AI system. |
taitelee
approved these changes
Sep 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR is no longer the computed-column change. All of that —
IsInsertable,InsertableColumns/InsertableColumnNames, the supplied-value400, the check-clause403, and the integration fixtures — moved into #554 during its review, because each piece was needed to keep #554 from being broken or silently unsafe on its own. #554 is CodeRabbit-approved and carries it.What is left here is a docs and test-hygiene layer on top of that:
api.mdbatch-ingest examples. A correctness fix, not decoration: the previous request bodies ({"page": "/about"}) omitted two non-nullable columns with no default, which the Schema Validation rules on the same page say are rejected — and the response example below them already blamed areferrerfield the request never sent. The examples now carry every required column, and the SSE example shows a projection.deployment.md— Bring Your Own Schema. WhatMATERIALIZED,ALIASandEPHEMERALmean for someone writing the DDL, which is a different audience and a different moment from the upgrade runbook further down.AGENTS.md— the numbered per-table-batching and DLQ points, which still described inserts in "schema column order" rather than the per-column-list grouping this stack introduced. Also qualifies the schema-frame guarantee, which was stated unconditionally against the#543known limitation.sdk/queries.md— aschema()example showingdefault_kind/default_expression.tests/integration/dlq_test.go— map literals to the typedingest.EventMessage. Behavior-preserving; the typed struct adds"scope":"", which the worker ignores.Merging #554 in also required removing eight duplicated claims that survived at non-conflicting offsets, where
-X theirscould not see them — including adefault_kindfield declared twice in one TypeScript interface, two doubled CHANGELOG entries, and a pair of bullets inapi.md's validation list whose bolded lead-ins flatly contradicted each other.Stacked PR
This is part 6 of 7 in a stack that replaces #540. Each PR is based on the one above it, so review this PR's own diff against its base — GitHub shows only this layer's changes.
stack/0-classify-pathsmainstack/1-discoverystack/0-classify-pathsstack/2-policystack/1-discoverystack/3-content-typestack/2-policystack/4-seamsstack/3-content-typestack/5-positional-wirestack/4-seamsstack/6-computed-columnsstack/5-positional-wireMerge in order, top to bottom. Rebasing or squashing out of order will make the later PRs' diffs unreadable.
Test plan
make cigreen on this branch's exact tree (verify, unit, integration against live ClickHouse, e2e, all coverage gates)go.mod/go.sumuntouched; no new dependenciesReview
Both pre-push reviewers gate the tip of the stack (#555), whose delta against
mainis the union of all seven branches. This is the branch both reviewers gate.🤖 Generated with Claude Code
https://claude.ai/code/session_018Epn88jTEw4ZkXrvTKzZXQ