Repository navigation
fix(sql): keep ClickHouse plugin macros intact when substituting $__from and $__to - #1346
arunms-ziwo wants to merge 1 commit into
Conversation
…rom and $__to The ClickHouse dialect replaced $__from, $__to and $__interval as plain substrings, so the plugin's own macros lost their prefix before reaching Grafana: $__toTime became 1792159528507Time (an unknown identifier), $__fromTime likewise, and $__interval_s became 3s_s. Any dashboard panel using them failed through run_panel_query and query_sql. Replace whole tokens only (a word boundary after the macro name), leaving $__fromTime, $__toTime, $__fromTime_ms, $__toTime_ms, $__interval_s, $__timeInterval and the other plugin macros for the plugin to expand.
sd2k
left a comment
sd2k
left a comment
There was a problem hiding this comment.
Thanks for this! The fix works for ClickHouse: $__fromTime, $__toTime, $__fromTime_ms, $__toTime_ms and $__interval_s now pass through untouched, and $__from/$__to next to punctuation still get substituted. The unit tests pass locally.
The main thing: the same bug is still in the other five dialects. Postgres, MySQL, MSSQL, Snowflake and Athena still do a plain strings.ReplaceAll for $__from / $__to / $__interval:
tools/sql/postgres.go:58tools/sql/mysql.go:58tools/sql/mssql.go:58tools/sql/snowflake.go:61tools/sql/athena.go:106
So a token that starts with one of those names (for example a variable like $__to_date) still gets corrupted to 1705316400000_date. Could you move replaceWholeMacro into tools/sql/dialect.go (next to EnforceSQLLimit / ValidateSQLIdentifier) and use it in every dialect? Then the fix lives in one shared place. A few small inline comments below as well.
| // replaceWholeMacro replaces every occurrence of macro that is not immediately followed by | ||
| // an identifier character, so $__to never matches the $__to prefix of $__toTime. | ||
| func replaceWholeMacro(query, macro, value string) string { | ||
| re := regexp.MustCompile(regexp.QuoteMeta(macro) + `\b`) |
There was a problem hiding this comment.
This compiles a new regex on every call, so each query_sql call compiles four. The macro names never change, so these could be precompiled once at package level, the way tools/dashboard_helpers.go does for substituteVariables. Another option is a single \$__(from|to|interval_ms|interval)\b regex with a lookup for the replacement values.
| return &DescribeSQLTableResult{Columns: result}, nil | ||
| } | ||
|
|
||
| // replaceWholeMacro replaces every occurrence of macro that is not immediately followed by |
There was a problem hiding this comment.
This is generic macro logic, not anything ClickHouse-specific. It'd fit better in dialect.go so the other dialects can use it too (see the review summary).
| "SELECT toStartOfInterval(ts, INTERVAL greatest($__interval_s, 60) SECOND) FROM logs", | ||
| }, | ||
| { | ||
| "$__from and $__to next to punctuation", |
There was a problem hiding this comment.
Two gaps in the tests:
$__timeInterval(col)/$__timeInterval_ms(col): the new comment inclickhouse.gosays they're protected, but no test covers them. A case would catch it if someone later makes the match case-insensitive or drops the\b.- The braced
${__from}/${__to}form: a case pinning down how it's handled (right now it isn't substituted at all) would make that behaviour explicit.
This branch has not been deployed
2 participants
What
The ClickHouse dialect's
SubstituteMacrosreplaced$__from,$__to,$__interval_msand$__intervalas plain substrings. The ClickHouse datasource plugin has macros that start with the same characters and are expanded server-side, so they reached Grafana already mangled:$__toTime→1792159528507Time(ClickHouse: unknown identifier)$__fromTime→1792155928507Time$__interval_s→3s_sAny panel using those macros failed through
run_panel_query(and the same SQL throughquery_sql). Observed on Grafana 12 withgrafana-clickhouse-datasourcepanels that use$__fromTime/$__toTimeandgreatest($__interval_s, 60).The substitution now replaces whole tokens only (word boundary after the macro name), leaving
$__fromTime,$__toTime,$__fromTime_ms,$__toTime_ms,$__interval_s,$__timeIntervaland the other plugin macros for the plugin.Tests
Four new cases in
TestClickHouseDialect_SubstituteMacros:$__fromTime/$__toTimeuntouched, the_msvariants untouched,$__interval_suntouched, and$__from/$__tonext to punctuation still replaced.go test -tags unit ./tools/sql,go vet,gofmtpass.Scoped to the ClickHouse dialect; the other dialects use the same substring replace but their plugins have no
$__from…/$__to…-prefixed macros.