Record the default.merges ClickHouse schema and the column #195503 needs - #8719
izaitsevfb wants to merge 1 commit into
Conversation
default.merges had no entry under clickhouse_db_schema/ at all, so the only description of its shape was the positional column list inside the replicator lambda. The CREATE here is `SHOW CREATE TABLE` verbatim, plus the one column pytorch/pytorch#195503 writes. The ALTER is a separate file rather than a comment because it has a sequencing constraint worth reading before anyone runs it: the replicator inserts positionally, so this ALTER on its own silently breaks every merge insert until the lambda declares the column too.
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
jeanschmidt
left a comment
jeanschmidt
left a comment
There was a problem hiding this comment.
worth having a look, but none are blocking IMO
Review summary
🟠 2 major · 🟡 1 minor · ⚪ 2 nit
- 🟠 The ALTER file's first sequencing option leaves ai_not_related_checks permanently empty because merges_adapter never declares the field —
clickhouse_db_schema/default.merges/001_alter_add_ai_not_related_checks.sql:11 - 🟠 The second sequencing option, applying the ALTER and the merges_adapter change "together", cannot be atomic and silently drops merge records in either order —
clickhouse_db_schema/default.merges/001_alter_add_ai_not_related_checks.sql:13 - 🟡 The new numbered ALTER file departs from clickhouse_db_schema/, where every table is one schema.sql, and duplicates the column schema.sql already declares —
clickhouse_db_schema/default.merges/001_alter_add_ai_not_related_checks.sql:20 - ⚪ "Applied to the live table by hand" reads as a claim the ALTER has already been run, which the rest of the file says would have broken merge ingestion —
clickhouse_db_schema/default.merges/001_alter_add_ai_not_related_checks.sql:19 - ⚪ schema.sql records a column the live table lacks, so creating default.merges from it yields a shape merges_adapter cannot insert into —
clickhouse_db_schema/default.merges/schema.sql:28
Extended analysis (ai-generated section)
What this changes
Two new SQL files under clickhouse_db_schema/default.merges/, no executable code. schema.sql records the table as a CREATE TABLE: the 20 columns that merges_adapter declares, name-for-name and order-for-order, plus _meta, and additionally ai_not_related_checks between unstable_checks and _meta. 001_alter_add_ai_not_related_checks.sql carries the matching ALTER behind a 19-line comment explaining that the replicator's positional insert makes the statement unsafe to apply alone. The snapshot therefore describes the table after that ALTER lands, not today's shape; the description states this and offers to split it.
What it gets right
The coupling the comment documents is real and verifiable in-repo: general_adapter builds insert into {table} select *, ('{bucket}', '{key}') as _meta from s3(...) at aws/lambda/clickhouse-replicator-s3/lambda_function.py:396-405, routes any exception to errors.gen_errors at 418-419, and lambda_handler only warns, so a column-count mismatch does lose merge records with nothing surfaced. AFTER unstable_checks is the right placement: it keeps _meta last, which is the invariant misc.runner_fleet_count/schema.sql:8-11 spells out for the other positionally-ingested table. Both files cross-reference each other.
Where the risk is
Everything lands on two spots: the SEQUENCING bullets at lines 10-13 of the ALTER file, and schema.sql:28. Independent passes converged on the bullets from four directions, and the two clusters there compound in a way neither shows alone — each of the two offered orders is wrong in the opposite direction. One cluster establishes that the #8716-first order removes the count break but leaves the column permanently empty, because select * expands over merges_adapter's s3() structure string, which never names the field; the placement observation about alphabetical order sits in that same cluster and is the same edit. The other cluster establishes that the "apply together" order cannot be atomic — hand-applied DDL against a CI-deployed lambda — so it has a live loss window in either direction. Read together: the safe order is incomplete and the complete order is lossy, and the file presents them as equivalent.
The schema.sql:28 cluster is one question asked five ways: a file the README defines as "a snapshot of the current shape" now leads reality, and the CREATE TABLE body is copy-pasteable in isolation into a 22-column table the deployed lambda cannot insert into. The convention finding about the numbered ALTER file rides on the same decision. The recurring class here is prose that is exact about the hazard it names and silent about the adjacent one.
Before merge, and after
Rewrite lines 10-13. "The ALTER file's first sequencing option leaves ai_not_related_checks permanently empty" and "The second sequencing option ... cannot be atomic" are one edit to that block, and the alphabetical-placement note belongs in it: state that merges_adapter's structure string must gain the field at its end in every order, and that only the #8716-first order has no loss window. The description's "Two orders work" needs the same correction. Two more cheap ones: "ADD COLUMN IF NOT EXISTS does not reposition an existing column" is a one-token change, and "Applied to the live table by hand" is a rewording.
The snapshot question — whether clickhouse_db_schema/default.merges/schema.sql records today's shape or the post-ALTER shape — is the call the description already asks for. It settles that whole cluster either way, and whichever way it goes the file wants a runner_fleet_count-style positional-insert warning; the two-files-one-column duplication resolves with it.
| -- merge insert on column count -- wherever the column is placed -- until that | ||
| -- lambda also declares it. The break is silent: the exception goes to | ||
| -- errors.gen_errors and the S3 objects are not reprocessed. So either | ||
| -- * land pytorch/test-infra#8716 first (it names the columns, after which |
There was a problem hiding this comment.
sounds something to check
🟠 The ALTER file's first sequencing option leaves ai_not_related_checks permanently empty because merges_adapter never declares the field (ai-generated section)
Lines 10-13 present two options as equally sufficient ("So either ... or ..."; the PR description says "Two orders work"). The second option includes "the matching merges_adapter schema change"; the first does not. But merges_adapter passes an explicit structure string to ClickHouse's s3() table function, and s3() parses only the fields named in that string — a JSON field the producer emits but the structure omits is never read. Naming the insert columns (option 1) removes the column-count error but does nothing to make the value arrive. aws/lambda/clickhouse-replicator-s3/lambda_function.py:323-344 declares exactly 20 backticked fields, ending at unstable_checks, and passes them to general_adapter at line 346; general_adapter builds from s3('{url}', '{format}', '{schema}', '{compression}', ...) at lines 400-404. Nothing in this diff touches that string. Line 3 of this same file states the column is "Written by save_merge_record in pytorch/pytorch#195503", i.e. trymerge does emit the field — the replicator would simply drop it. Whoever follows the first documented order applies the ALTER, sees merge ingestion keep working, and concludes the column is live. Every row lands with ai_not_related_checks = [], so the AI-advisor audit trail this column exists to create records nothing, and the gap surfaces only when someone queries the column weeks later and finds it uniformly empty. State the merges_adapter structure-string change as required in every order, with only its timing varying: replace the "So either / or" framing with a single sentence — "merges_adapter's s3() structure string must gain ai_not_related_checks in all cases; #8716 only decides whether the ALTER can land before it or must land with it." The same correction is needed in the PR description's "Two orders work" list. The narrow alternative is to append "...then apply this ALTER -- the column stays empty until merges_adapter declares it" to the first bullet; I would take the broader rewording, since the bullet list is what a reader executes.
Reviewed by claude-opus-5[1m] at max effort, against a6e5fde.
There was a problem hiding this comment.
is you agent aware that this PR has a counterpart?
| -- errors.gen_errors and the S3 objects are not reprocessed. So either | ||
| -- * land pytorch/test-infra#8716 first (it names the columns, after which | ||
| -- the table's column count stops mattering), or | ||
| -- * apply this ALTER and the matching merges_adapter schema change together. |
There was a problem hiding this comment.
the deployment needs to be performed in a quiet sunday window
🟠 The second sequencing option, applying the ALTER and the merges_adapter change "together", cannot be atomic and silently drops merge records in either order (ai-generated section)
The file offers two orderings as equally workable. The first (land the named-column insert first) is genuinely windowless. The second is not: the ALTER runs by hand against ClickHouse while the lambda ships through CI, so the two steps are minutes apart in whichever order they happen, and during that gap the positional insert's value count disagrees with the table's column count. ALTER first gives 21 values against 22 columns; lambda first gives 22 values against 21. Both raise NUMBER_OF_COLUMNS_DOESNT_MATCH on every merge object, and the file's own paragraph above explains that those objects are lost rather than retried. merges_adapter declares 20 columns (aws/lambda/clickhouse-replicator-s3/lambda_function.py:324-343) and calls general_adapter, whose query is insert into {table} select *, ('{bucket}', '{key}') as _meta from s3(...) (lambda_function.py:396-405) — exactly 21 values. After this ALTER the table has 22 ordinary columns (schema.sql:8-31). general_adapter catches the exception and routes it to log_failure_to_clickhouse (lambda_function.py:418-419), and lambda_handler only warns (lambda_function.py:47-48), so nothing propagates to S3 and the notification is never retried. The lambda is deployed by CI on push to main (aws/lambda/clickhouse-replicator-s3/README.md:8-9, .github/workflows/clickhouse-replicator-s3-lambda.yml:3-9,32), so there is no moment at which "together" is simultaneous. An operator who picks the second bullet, reading it as safe because it is presented alongside the safe one, permanently loses every merge record trymerge uploads during the CI deploy window — a handful of rows on a busy day, with no error surfaced anywhere except a row in errors.gen_errors. Missing rows are unrecoverable for tools/flake-test-fail-autorevert (which reports NOT_RUN:no_merge_record) and torchci/clickhouse_queries/pr_merge_commits. Narrow fix: delete the second bullet and state that landing pytorch/test-infra#8716 first is the only order with no loss window. Broader fix: keep the second bullet but spell out that it is not atomic — the ALTER and the lambda deploy are minutes apart in either direction, every merge object uploaded in that gap lands in errors.gen_errors and must be re-ingested by hand afterwards. I would take the narrow fix; the whole point of the file is to stop someone losing records.
Reviewed by claude-opus-5[1m] at max effort, against a6e5fde.
| -- assumes. | ||
| -- | ||
| -- Applied to the live table by hand -- see clickhouse_db_schema/README.md. | ||
| ALTER TABLE default.merges |
There was a problem hiding this comment.
probably not intentional?
🟡 The new numbered ALTER file departs from clickhouse_db_schema/, where every table is one schema.sql, and duplicates the column schema.sql already declares (ai-generated section)
This file introduces a numbered-migration convention into clickhouse_db_schema/, a directory whose README states each table is declared once as a per-table schema.sql, and where none of the 20+ table directories contains a migration file. It also restates ai_not_related_checks Array(Array(String)) (line 21), which the sibling schema.sql:28 added by the same change already declares, so the directory now carries one column definition twice under two different conventions. clickhouse_db_schema/README.md:11-13: "Each table is declared once as a per-table schema.sql (a snapshot of the current shape), optionally alongside a grants.sql for its permissions. There is no automated upstreaming." Every sibling follows it: clickhouse_db_schema/default.workflow_job/ and clickhouse_db_schema/misc.oss_ci_queue_time_histogram/ hold only schema.sql; clickhouse_db_schema/misc.autorevert_advisor_verdicts/ holds schema.sql plus grants.sql (grants.sql:2). Columns plainly added by a later ALTER are recorded by editing the snapshot in place, with no ALTER file: default.workflow_job/schema.sql:40 (backfill Bool DEFAULT true) and :45 (torchci_classification_temp, sitting after the ALIAS columns). The exact constraint this new file exists to carry is already recorded in-repo as a comment inside the table's own snapshot: clickhouse_db_schema/misc.runner_fleet_count/schema.sql:8-11 ("the general_adapter insert is positional (SELECT *, _meta), so the column order here MUST match the adapter's schema string exactly, with _meta last"). The numbered-ALTER convention this file copies lives in greenlight/sql/ (002_, 003_, 004_), and there the base CREATE is deliberately not edited: greenlight/sql/001_create_misc_greenlight_pr_state.sql:21-34 still lacks _meta, which greenlight/sql/003_alter_greenlight_pr_state_add_meta.sql:6 adds, with a forward-pointing note at 001_create_misc_greenlight_pr_state.sql:1-5 instead. This change does both at once: it edits the snapshot (default.merges/schema.sql:28) and adds the ALTER. clickhouse_db_schema/default.merges/ now has two hand-applied files that must agree on the column's type and position, with nothing checking them. Changing Array(Array(String)) in one leaves the other silently wrong, and the disagreement only surfaces when someone runs the stale file against the live cluster. The next person adding a column to default.merges has no rule to follow -- edit schema.sql, add 002_alter_..., or both -- and every choice leaves the directory inconsistent either with itself or with the other 20 tables. Narrow (what I would do, because it is what the directory already does): delete 001_alter_add_ai_not_related_checks.sql and move its SEQUENCING block into a header comment on clickhouse_db_schema/default.merges/schema.sql, in the same shape as clickhouse_db_schema/misc.runner_fleet_count/schema.sql:8-11; whoever applies it derives the one-line ALTER TABLE default.merges ADD COLUMN ... from the snapshot, and the column type then exists in exactly one place. Broader: if executable migrations are wanted for clickhouse_db_schema/ tables, add them under a layout the README describes and stop editing schema.sql for altered columns, matching how greenlight/sql/001_create_misc_greenlight_pr_state.sql is left untouched by 002-004.
Reviewed by claude-opus-5[1m] at max effort, against a6e5fde.
| -- is what the positional insert requires and what the rest of this directory | ||
| -- assumes. | ||
| -- | ||
| -- Applied to the live table by hand -- see clickhouse_db_schema/README.md. |
There was a problem hiding this comment.
nit, should be an easy fix
⚪ "Applied to the live table by hand" reads as a claim the ALTER has already been run, which the rest of the file says would have broken merge ingestion (ai-generated section)
Line 19 is a subjectless past participle with two opposite readings: "this has been applied to the live table" or "files here are applied by hand, not by automation". The file's own SEQUENCING block makes the difference material rather than cosmetic. Lines 7-10 of this file: "Running this ALTER on its own therefore breaks EVERY merge insert on column count -- wherever the column is placed -- until that lambda also declares it. The break is silent". The PR description places the application in the future ("schema.sql here describes the table after the sibling ALTER is applied"), and the diff does not touch merges_adapter in aws/lambda/clickhouse-replicator-s3/lambda_function.py:322-346, so under the file's own analysis it cannot have been applied without breaking ingestion. A reader taking the first reading believes the column already exists live and skips the sequencing they were just warned about, or is simply misled about the current state of the table while triaging a merge-ingestion problem. Rephrase to the process reading, e.g. "-- Apply by hand; there is no automation for this directory -- see clickhouse_db_schema/README.md." If it genuinely has already been applied, say so explicitly with a date and which of the two orders was used, and update the schema.sql header accordingly.
Reviewed by claude-opus-5[1m] at max effort, against a6e5fde.
| `project` String, | ||
| `skip_mandatory_checks` Bool, | ||
| `unstable_checks` Array(Array(String)), | ||
| `ai_not_related_checks` Array(Array(String)), |
There was a problem hiding this comment.
nit, should be an easy fix
⚪ schema.sql records a column the live table lacks, so creating default.merges from it yields a shape merges_adapter cannot insert into (ai-generated section)
clickhouse_db_schema/README.md:11-13 defines schema.sql as "a snapshot of the current shape" that is applied to ClickHouse by hand. This snapshot includes ai_not_related_checks, which by the sibling ALTER's own account is not yet on the table, and the header comment (lines 4-5) discloses the discrepancy without carrying the positional-insert hazard that makes it dangerous. Anyone using the file for its stated purpose — a fresh cluster, a staging table, a restore — gets a 22-column table. merges_adapter (aws/lambda/clickhouse-replicator-s3/lambda_function.py:322-346) declares 20 columns and general_adapter emits select *, ('{bucket}', '{key}') as _meta (lines 397-405), i.e. 21 values, against the 22 columns this file creates. The mismatch is caught at lines 418-419 and routed to errors.gen_errors, so the Lambda returns success and the object is not redelivered. The sibling snapshot for the other positionally-ingested table carries exactly this warning inline — clickhouse_db_schema/misc.runner_fleet_count/schema.sql:8-11: "IMPORTANT: the general_adapter insert is positional (SELECT *, _meta), so the column order here MUST match the adapter's schema string exactly" — and this file has no equivalent. A table created from this file today silently accepts zero merge records: every insert lands in errors.gen_errors instead, and nothing in the pipeline reports failure. The operator sees an empty default.merges and has to trace it back through the replicator to a column-count error. Narrow (what I would pick): keep the column but add the positional-insert warning to the header, in the shape of clickhouse_db_schema/misc.runner_fleet_count/schema.sql:8-11 — state that the live table does not have this column yet and that creating the table from this file requires merges_adapter to declare it too. Broader: record only today's shape, drop line 28, and add the column here when the ALTER and the adapter change land.
Reviewed by claude-opus-5[1m] at max effort, against a6e5fde.
This branch has not been deployed
3 participants
✴️ iz2: opened on behalf of @izaitsevfb
default.mergeshas no entry underclickhouse_db_schema/, so the only written description of its shape is the positional column list insideaws/lambda/clickhouse-replicator-s3/lambda_function.py. This adds one.schema.sqlSHOW CREATE TABLE default.mergesverbatim — diffed against the live cluster, byte-identical apart from the one column below — plusai_not_related_checks, whichsave_merge_recordwrites in pytorch/pytorch#195503.001_alter_add_ai_not_related_checks.sqlA separate file rather than a comment, because it carries a constraint worth reading before anyone runs it.
The replicator lambda inserts positionally (
insert into default.merges select *, (bucket, key)), so it supplies exactly as many values as the table has columns. Running this ALTER on its own breaks every merge insert on column count — wherever the column is placed — until the lambda declares it too. It breaks silently:general_adapterroutes the exception toerrors.gen_errorsand the S3 objects are not reprocessed, so those merge records are lost.Two orders work:
merges_adapterschema change together.AFTER unstable_checkseither way, so_metastays last — what the positional insert needs and what the rest of this directory assumes.Note on the snapshot being slightly ahead of reality
Per
clickhouse_db_schema/README.mdthese files are applied to ClickHouse by hand and there is no automation, soschema.sqlhere describes the table after the sibling ALTER is applied. If you would rather it record only today's shape and be updated afterwards, say so and I will split it.