profit/golangLAKEHOUSE
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

G0 D5: queryd DuckDB SELECT over Parquet via httpfs · 4 scrum fixes

Browse Source 
Phase G0 Day 5 ships queryd: in-memory DuckDB with custom Connector
that runs INSTALL httpfs / LOAD httpfs / CREATE OR REPLACE SECRET
(TYPE S3) on every new connection, sourced from SecretsProvider +
shared.S3Config. SetMaxOpenConns(1) so registrar's CREATE VIEWs and
handler's SELECTs serialize through one connection (avoids cross-
connection MVCC visibility edge cases).

Registrar.Refresh reads catalogd /catalog/list, runs CREATE OR
REPLACE VIEW "name" AS SELECT * FROM read_parquet('s3://bucket/key')
per manifest, drops views for removed manifests, skips on unchanged
updated_at (the implicit etag). Drop pass runs BEFORE create pass so
a poison manifest can't block other manifest refreshes (post-scrum
C1 fix).

POST /sql with JSON body {"sql":"…"} returns
{"columns":[{"name":"id","type":"BIGINT"},…], "rows":[[…]],
"row_count":N}. []byte → string conversion so VARCHAR rows
JSON-encode as text. 30s default refresh ticker, configurable via
[queryd].refresh_every.

Cross-lineage scrum on shipped code:
  - Opus 4.7 (opencode):                      1 BLOCK + 4 WARN + 4 INFO
  - Kimi K2-0905 (openrouter):                2 BLOCK + 2 WARN + 1 INFO
  - Qwen3-coder (openrouter):                 2 BLOCK + 1 WARN + 1 INFO

Fixed (4):
  C1 (Opus + Kimi convergent): Refresh aborts on first per-view error
    → drop pass first, collect errors, errors.Join. Poison manifest
    no longer blocks the rest of the catalog from re-syncing.
  B-CTX (Opus BLOCK): bootstrap closure captured OpenDB's ctx →
    cancelled-ctx silently fails every reconnect. context.Background()
    inside closure; passed ctx only for initial Ping.
  B-LEAK (Kimi BLOCK): firstLine(stmt) truncated CREATE SECRET to 80
    chars but those 80 chars contained KEY_ID + SECRET prefix → log
    aggregator captures credentials. Stable per-statement labels +
    redactCreds() filter on wrapped DuckDB errors.
  JSON-ERR (Opus WARN): swallowed json.Encode error → silent
    truncated 200 on unsupported column types. slog.Warn the failure.

Dismissed (4 false positives):
  Qwen BLOCK "bootstrap not transactional" — DuckDB DDL is auto-commit
  Qwen BLOCK "MaxBytesReader after Decode" — false, applied before
  Kimi BLOCK "concurrent Refresh + user SELECT deadlock" — not a
    deadlock, just serialization, by design with 10s timeout retry
  Kimi WARN "dropView leaves r.known inconsistent" — current code
    returns before the delete; the entry persists for retry

Critical reviewer behavior: 1 convergent BLOCK between Opus + Kimi
on the per-view error blocking, plus two independent single-reviewer
BLOCKs (B-CTX, B-LEAK) that smoke could never have caught. The
B-LEAK fix uses defense-in-depth: never pass SQL into the error
path AND redact known cred values from DuckDB's own error message.

DuckDB cgo path: github.com/duckdb/duckdb-go/v2 v2.10502.0 (per
ADR-001 §1) on Go 1.25 + arrow-go. Smoke 6/6 PASS after every
fix round.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
root
2026-04-29 00:10:55 -05:00
co-authored by Claude Opus 4.7
parent 4205ecd0f0
commit 9e9e4c26a4
10 changed files with 1129 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/PHASE_G0_KICKOFF.md
+169
View File
@@ -825,3 +825,172 @@ data getting overwritten before the 409 fired. Opus reading the
PUT-then-register order and noticing that PUT mutates state before
the validation check is exactly the kind of architectural code-read
the scrum exists for.
---
## Pre-D5 prep (commit 4205ecd)
Between D4 and D5, J asked what would bite D5 if not fixed first.
The answer was the `CatalogClient` package location: it lived in
`internal/ingestd/` but D5's queryd needed the same shape, and
having queryd import from ingestd would have inverted the data-flow
direction (ingestd is upstream of queryd).
Extracted to `internal/catalogclient/` as a shared package, added
the `List(ctx)` method queryd needs for view registration, kept
the wire format unchanged. ingestd switched its import; all four
existing smokes (D1-D4) passed unchanged.
DuckDB cgo path re-verified at this point with the official
`github.com/duckdb/duckdb-go/v2` v2.10502.0 (per ADR-001 §1) on
Go 1.25 + arrow-go: standalone `sql.Open("duckdb","")` →
`db.Ping()` → `SELECT 'pong'` round-trip succeeded.
---
## D5 — actual run results (2026-04-29)
Phase G0 Day 5 executed end-to-end. Output of `scripts/d5_smoke.sh`:
```
[d5-smoke] ingest 5-row CSV via D4 path:
✓ ingest row_count=5
[d5-smoke] launching queryd (initial Refresh picks up d5_workers)...
[d5-smoke] POST /sql SELECT count(*) FROM d5_workers:
✓ count(*)=5
[d5-smoke] POST /sql SELECT * FROM d5_workers LIMIT 3:
✓ rows[0] = (id=1, name=Alice), columns=[id, name, salary]
[d5-smoke] schema-drift ingest 409s; existing view still queries:
✓ drift → 409
✓ post-drift count(*)=5 (view unchanged)
[d5-smoke] error path: SELECT FROM nonexistent → 400:
✓ unknown table → 400
[d5-smoke] D5 acceptance gate: PASSED
```
What landed:
- `internal/queryd/db.go` — `OpenDB(ctx, s3, prov, bucket)` returns
a `*sql.DB` backed by an in-memory DuckDB. Custom Connector with
a bootstrapper that runs `INSTALL httpfs` / `LOAD httpfs` /
`CREATE OR REPLACE SECRET (TYPE S3, KEY_ID …, ENDPOINT …,
URL_STYLE 'path', USE_SSL false)` on every new physical connection.
`SetMaxOpenConns(1)` so registrar's CREATE VIEWs and handler's
user SQL serialize through one connection (avoids cross-connection
visibility edge cases).
- `internal/queryd/registrar.go` — `Registrar.Refresh(ctx)` reads
catalog list, runs `CREATE OR REPLACE VIEW "name" AS SELECT *
FROM read_parquet('s3://bucket/key')` per manifest, drops views
for removed manifests, skips on unchanged `updated_at`.
`CatalogLister` + `Execer` interfaces enable unit tests with no
DuckDB / no real catalogd.
- `cmd/queryd/main.go` — POST /sql with JSON body
`{"sql":"…"}` → executes via `*sql.DB` → response shape
`{"columns":[{"name","type"},...], "rows":[[...]], "row_count":N}`.
`[]byte` → string conversion so VARCHAR rows JSON-encode as text.
Initial `Refresh` on startup, then a TTL ticker (`30s` default,
configurable via `[queryd].refresh_every`) calling `Refresh` in a
goroutine cancellable via process ctx.
- New `[queryd]` config block: `bind` + `catalogd_url` +
`secrets_path` + `refresh_every`.
- `scripts/d5_smoke.sh` — 6 acceptance probes including post-drift
query stability (`view unchanged after schema-drift 409`).
The D5 smoke launches queryd LAST — after the D4 ingest has
registered the dataset — so the initial `Refresh` picks it up
without waiting for the TTL. This matches what real ops looks like:
queryd starts after data is loaded, picks up the catalog at that
moment, then drifts in lockstep via the ticker.
Critical architectural choices:
1. **`SetMaxOpenConns(1)`** — DuckDB views live in the in-memory
catalog of a single instance; with one connection, registrar's
CREATE VIEWs and handler's SELECTs are visibly synchronized
without MVCC-timing edge cases. Lift in G2+ if intra-process
query concurrency wins matter.
2. **Always-quote view identifiers** — `CREATE OR REPLACE VIEW "name"`
with internal `"` doubled. Catalogd accepts user-supplied names
that wouldn't pass SQL bare-identifier rules; quoting makes them
unambiguous.
3. **`updated_at` as the implicit etag** — no separate ETag header
in catalogd. The manifest's timestamp bumps on every same-fp
re-register; registrar tracks `time.Time` per name and skips
`CREATE` when unchanged (Opus's "perf cliff" warning from D5 plan).
Next: D6 — gateway as a reverse proxy in front of all four backing
services. Last day of G0. After that: G1+ (gRPC, Lance bench, vector
indices, etc).
---
## D5 — code scrum review (3-lineage parallel pass)
| Pass | Reviewer | Latency | Findings |
|---|---|---|---|
| 1 | `opencode/claude-opus-4-7` | ~28s | 1 BLOCK + 4 WARN + 4 INFO = 9 (1 self-retracted) |
| 2 | `openrouter/moonshotai/kimi-k2-0905` | ~25s | 2 BLOCK + 2 WARN + 1 INFO = 5 |
| 3 | `openrouter/qwen/qwen3-coder` | ~20s | 2 BLOCK + 1 WARN + 1 INFO = 4 |
Total: 18 distinct findings. Strongest scrum yet on convergence
quality — Opus's BLOCK on ctx capture + Kimi's BLOCK on credential
leakage in error messages were both deep architectural reads that
smoke could not have caught.
### Convergent findings (≥2 reviewers — high confidence)
| # | Severity | Finding | Reviewers | Disposition |
|---|---|---|---|---|
| C1 | WARN×2 | `Refresh` aborts mid-loop on first per-view error → poison manifest blocks all subsequent drops/updates → stale views stick around forever (Opus); a partial `dropView` failure would block other manifest refreshes (Kimi) | Opus + Kimi | **Fixed** — drop pass runs BEFORE create pass; per-iteration errors are collected and Refresh continues; final return uses `errors.Join` to surface every failure |
### Single-reviewer findings — applied
| # | Severity | Finding | Reviewer | Disposition |
|---|---|---|---|---|
| B-CTX | Opus BLOCK | Bootstrap closure captures the `ctx` passed to `OpenDB`. Today masked by `SetMaxOpenConns(1)` + long-lived `procCtx`; future short-lived ctx + reconnect would silently fail every reconnect's bootstrap with a cancelled ctx | Opus | **Fixed** — bootstrap explicitly uses `context.Background()`; passed `ctx` is only used for the initial `PingContext` |
| B-LEAK | Kimi BLOCK | The prior `firstLine(stmt)` truncation to 80 chars STILL contained both `KEY_ID '...'` AND the start of `SECRET '...'` — log aggregators would capture credentials | Kimi | **Fixed** — replaced `firstLine(stmt)` with stable per-statement labels (`"install httpfs"` / `"load httpfs"` / `"create secret"`) so no SQL reaches the error path. Wrapped `err.Error()` is filtered through `redactCreds` to scrub any secret values DuckDB itself might echo |
| JSON-ERR | Opus WARN | `_ = json.NewEncoder(w).Encode(resp)` swallows mid-encode failures; client sees truncated 200 with no log signal — first column type DuckDB can't JSON-encode (e.g. INTERVAL) lands here silently | Opus | **Fixed** — log via `slog.Warn` with the underlying error |
### Single-reviewer findings — accepted with rationale
| # | Severity | Finding | Reviewer | Disposition |
|---|---|---|---|---|
| O-W3 | Opus WARN | DuckDB exotic types (Decimal, INTERVAL, etc.) may not JSON-encode well — `dest := []any` with `[]byte→string` conversion is the only type-aware step today | Opus | **Accepted** — type-aware row converter is post-G0; G0 column types in test fixtures are int64/varchar/double/bool. The new `JSON-ERR` fix makes any encoding failure operator-visible in the meantime |
| K-W1 | Kimi WARN | No exponential backoff on repeated refresh failures | Kimi | **Accepted** — G0 single-tenant; 30s ticker is fine. Backoff adds complexity that smoke can't validate |
| O-INFO×3 | Opus INFO | USE_SSL casing cosmetic; initial-refresh log doesn't match runRefreshLoop's four-field format | Opus | **Accepted** / **fixed cleanup** — the `var _ = io.Discard` placeholder removed; the rest deferred |
| Q-W | Qwen WARN | Object key validation | Qwen | **Accepted** — catalogd's contract; storaged validateKey is the actual gate |
| Q-I | Qwen INFO | Hardcoded refresh timeout | Qwen | **Accepted** — separate config knob is over-engineering for G0 |
### Dismissed (false positives)
| # | Severity | Finding | Reviewer | Why dismissed |
|---|---|---|---|---|
| F1 | Q-BLOCK | "Bootstrap not transactional" | Qwen | False — DuckDB DDL is auto-commit + idempotent; `CREATE OR REPLACE` handles re-run cleanly |
| F2 | Q-BLOCK | "MaxBytesReader applied AFTER json.NewDecoder reads" | Qwen | False — `cmd/queryd/main.go` sets `r.Body = http.MaxBytesReader(...)` BEFORE the decode call, by line ordering |
| F3 | K-BLOCK | "Concurrent Refresh + user query deadlock on single connection" | Kimi | False — not a deadlock, just serialization. With Refresh's 10s ctx-timeout, slow SELECT causes Refresh to skip a tick and retry. Design accept |
| F4 | K-W2 | "dropView failure leaves r.known inconsistent" | Kimi | False — current code returns BEFORE the `delete(r.known, name)` on dropView error, so the entry persists and next refresh retries (correct behavior). Misread of the sequence |
### Cumulative D5 disposition
- **3-lineage parallel pass: 18 distinct findings**
- **Fixed: 4** (1 convergent + 3 single-reviewer real)
- **Accepted-with-rationale: 5**
- **Deferred / cleanup: 3** (1 INFO removal, 2 INFOs deferred)
- **Dismissed (false positives): 4**
- Build clean, vet clean, all tests pass, smoke 6/6 PASS after every fix round.
The B-LEAK finding (Kimi BLOCK) is worth a pause: the prior code
explicitly tried to be careful about credentials by truncating the
SQL to 80 chars in error messages, but the truncation window still
captured both `KEY_ID '<value>'` and the prefix of
`SECRET '<value>'` because of where they appear in the statement.
The defense-in-depth answer was to never pass the SQL into the
error path at all — labels are static, the error chain wraps a
credential-redacted version of DuckDB's own error message. Two
layers of "don't leak" instead of one fragile truncation.
The B-CTX finding (Opus BLOCK) is the kind of latent issue that
would cost a future debugger hours: the connector callback runs on
EVERY new physical connection, but `SetMaxOpenConns(1)` masks the
issue today. A future change to allow concurrent connections would
silently break bootstrap on every reconnect with no clear error
signal. Catching it during the scrum saved a Friday-afternoon-
debugging incident months from now.