Repository navigation
feat: add explicit --collectdatabase option - #15
Conversation
A production collect Job created its auxiliary table in the wrong ClickHouse database: database qualification lived only inside COLLECTTABLEPREFIX as an embedded "db.prefix_" string, and it is easy to omit that dot by accident. A second, independent bug in the same code path meant a mismatch warning never fired when the table lived outside the user's current database. Acceptance criteria: - A new --collectdatabase/S3GC_COLLECTDATABASE option qualifies the auxiliary/run-log tables' database without embedding it in COLLECTTABLEPREFIX. - Given both, a conflicting pair fails before any ClickHouse or S3 call, naming both values, instead of silently picking one. - Given both with the same database, or COLLECTTABLEPREFIX alone, behavior is unchanged from before this change. - check_samples_match_partitioning() checks the actual configured database, not always currentDatabase(), so the --samples mismatch warning fires regardless of which database the auxiliary table lives in. - The Kubernetes renderer accepts an optional COLLECTDATABASE value and wires S3GC_COLLECTDATABASE into the Job only when set. - example.env and both READMEs recommend COLLECTDATABASE as the primary way to qualify a prefix; the embedded db.prefix_ form remains documented and supported for existing deployments. Also removes a redundant, unused re-split of COLLECTTABLEPREFIX in do_collect() found while consolidating this logic. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa78fd1d12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rows = ch_client.query( | ||
| "SELECT partition_key FROM system.tables " | ||
| f"WHERE database = currentDatabase() AND name = '{tname.strip('`').split('.')[-1]}'" | ||
| f"WHERE {database_filter} AND name = '{tname.strip('`').split('.')[-1]}'" |
There was a problem hiding this comment.
When --collectdatabase or an embedded database qualifies the table, tname has the form `aux_db`.`s3gc_run_s3`, so tname.strip('').split('.')[-1]evaluates to ``s3gc_run_s3 `` with a leading backtick. The system.tables query therefore finds no row and silently skips the `--samples` mismatch warning, meaning the fix described by this commit still does not work for the newly recommended configuration; derive the unquoted table component separately before building this predicate.
Useful? React with 👍 / 👎.
check_samples_match_partitioning() derived the table name by stripping backticks from a qualified tname before splitting on the dot, so `aux_db`.`s3gc_run_s3` became "`s3gc_run_s3" with a stray backtick. The system.tables lookup then found no row and the --samples mismatch warning was skipped silently -- in exactly the COLLECTDATABASE configuration this branch recommends. Reported by Codex review on PR #15. Acceptance criteria: - With a database-qualified auxiliary table, the system.tables lookup matches on the unquoted table name, and a --samples mismatch warns. - An unqualified auxiliary table keeps working unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Summary
A production collect Job created its auxiliary table in the wrong ClickHouse database, because database qualification lived only inside
COLLECTTABLEPREFIXas an embeddeddb.prefix_string — easy to omit by accident. This was HANDOFF.md item #1 from that incident triage.--collectdatabase/S3GC_COLLECTDATABASEto qualify the auxiliary/run-log tables' database explicitly (s3gc.py:245-259, prefix-resolution block ats3gc.py:677-706). A conflicting pair (differs from a database embedded inCOLLECTTABLEPREFIX) fails fast with both values named. Given both with agreement, orCOLLECTTABLEPREFIXalone, behavior is unchanged — no breaking change for existing deployments.check_samples_match_partitioning()(s3gc.py:1219+): it always checkedsystem.tables WHERE database = currentDatabase(), so the--samplesmismatch warning silently never fired once the auxiliary table lived in a different database. This was a second, independent bug the same incident exposed.COLLECTTABLEPREFIXindo_collect()found while consolidating this logic.COLLECTDATABASEis optional, wired toS3GC_COLLECTDATABASEonly when set (render.py,job.yaml.tmpl).example.envand both READMEs now recommendCOLLECTDATABASEas the primary way to qualify a prefix; the embeddeddb.prefix_form stays documented and supported.Spec-first TDD per
CLAUDE.md: acceptance criteria are in the commit message, each covered by a test written to fail before the implementation (git log -1for the full spec).Test plan
pytest -v -m "not dev_cluster"— 120 passed (112 existing + 8 new)render.pyrendersexample.envcleanly withS3GC_COLLECTDATABASEpresentkubeconform -strictvalidates the rendered manifest🤖 Generated with Claude Code