🔨🤖 Store ETL chart config as a separate config row (rows model) + catalogPath#6599
Conversation
Replaces the chart_configs.etlConfig column with a charts.configIdETL pointer to a dedicated chart_configs row, mirroring variables.grapherConfigIdETL. Keeps chart_configs a generic config store and lets each owner declare its own layers via pointers (per Sophia's feedback). The rendered full is still merge(variableETL, etlConfig, patch); the ETL config row is MySQL-only and never uploaded to R2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ges) Adds a nullable, unique catalogPath column to the charts table linking an ETL-authored chart to its ETL step (e.g. animal_welfare/latest/banning_of_chick_culling#banning_of_chick_culling), mirroring multi_dim_data_pages.catalogPath. Persisted via the etlConfig endpoint (COALESCE so non-ETL callers don't clobber it). NULL for hand-authored charts. Pairs with the ETL-side change. Targets the chart-config feature branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
![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: 0c12779bfc
ℹ️ 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".
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88570ec992
ℹ️ 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".
2 participants
Subbranch PR into
etl-chart-config. Reworks how a chart's ETL-authored config is stored, from a column onchart_configsto a separate config row reached by a pointer — and folds in thecatalogPathchange (supersedes #6590).Why
Following Sophia's feedback on #6553: keep
chart_configsa generic config store, and let each owner declare its own layers via pointers (exactly howvariables.grapherConfigIdETLalready works). This preserves "one row = one layer", reuses the existing config-row machinery (validation, R2, merge), and avoids baking a chart-specific concept into the sharedchart_configstable.What changes vs the column approach
chart_configs.etlConfigcolumn; addcharts.configIdETL char(36) UNIQUE NULL→ FK tochart_configs(id), mirroringvariables.grapherConfigIdETL. The chart's ETL config lives in its ownchart_configsrow (patch == full, absolute).fullis stillmerge(variableETL, etlConfig, patch). The middle layer is now read through the pointer (LEFT JOIN chart_configs cc_etl ON cc_etl.id = c.configIdETL) — in the admin-save path,parent.json, and the variable cascade (updateAllChartsThatInheritFromIndicator).PUT /charts/:id/etlConfigupserts the separate ETL config row and sets the pointer;DELETEclears the pointer then deletes the row.deleteChartalso drops the orphaned ETL row. API request/response shapes are unchanged, so the admin editor needs no changes.Folded in:
catalogPath(was #6590)Adds
charts.catalogPath(ETL's stable identity, mirrorsmulti_dim_data_pages.catalogPath) and wires it into the ETL-config endpoint. Brought in here so the whole charts-table reshape is one reviewable PR; #6590 is superseded.Testing
yarn typecheckpasses.adminSiteServer/tests/charts.test.ts) and the migrations need a MySQL test DB — to be validated in CI (make dbtest). The two storage-specific assertions were updated to check the separate row viaconfigIdETL. Logic closely mirrors the existing, tested variable-ETL pattern.🤖 Generated with Claude Code