fix: dictionary replace with staged models not working - #9828
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where refreshing a ClickHouse dictionary model with staged changes (stage_changes: true) fails or leaks source tables. ClickHouse forbids dropping or renaming a table that a dictionary depends on, and the previous implementation derived the dictionary's source ("temp") table name deterministically from the dictionary's current name (<name>_dict_temp_). When a dictionary was staged under __rill_tmp_model_<name> and later renamed, that reconstructed name no longer matched the real source table, so cleanup silently failed and stale tables accumulated (and a refresh could not replace the in-use source table).
The fix writes each refresh to a uniquely-named source table (<name>_dict_temp_<uuid>), repoints the dictionary at it via CREATE OR REPLACE DICTIONARY, and then discovers/drops the previous source table by reading system.tables.loading_dependencies_table rather than reconstructing the name. This keeps the dictionary serving until the new data is ready and correctly garbage-collects the old source table across create/refresh/rename flows.
Changes:
- Rework
createDictionary/dropTableto use unique per-refresh source tables and resolve the current source table vialoading_dependencies_table, with best-effort cleanup guarded by arepointedflag. - Extract a shared
stagingTablePrefixconstant so the source table name can strip the staging prefix and stay stable/readable. - Refactor the ClickHouse self-tests onto a shared instance (
newInstancewith per-test databases) and add coverage for staged dictionary refresh, schema-changing refresh, dictionary model rename, and preservation of user-managed source tables.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
runtime/drivers/clickhouse/crud.go |
Core fix: unique source-table naming, dependency-based source resolution, deferred cleanup, and repoint-then-drop logic for dictionaries. |
runtime/drivers/clickhouse/model_manager.go |
Introduces the shared stagingTablePrefix constant used by the source-table naming logic. |
runtime/drivers/clickhouse/model_executor_self_test.go |
Consolidates tests under a shared ClickHouse instance and adds staged-refresh/rename dictionary coverage. |
runtime/drivers/clickhouse/olap_test.go |
Adds coverage that dropping a dictionary leaves user-managed (non-Rill) source tables intact. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| // first drop the dictionary | ||
| err := c.Exec(ctx, &drivers.Statement{ | ||
| err = c.Exec(ctx, &drivers.Statement{ |
There was a problem hiding this comment.
Do we need to handle this err, if its non nil then we still drop source table later and that will error too? Thats why no need to handle it? may be log it?
| // newDictionarySourceTable returns a unique name for a table for the dictionary `name` to source from. | ||
| // The name is unique per call because ClickHouse forbids dropping or renaming a table that a dictionary depends on, | ||
| // so refreshing a dictionary has to write to a new table and repoint the dictionary at it. | ||
| func newDictionarySourceTable(name string) string { |
There was a problem hiding this comment.
Since this produces unique names now as opposed to our earlier fixed named, can we check there are no orphaned tables left behind in edge case. May be ask AI to find any leakage paths, but they sometimes find very impractical or complex cases which I think can be ignored and just rely on logging it properly if it happens.
Checklist: