Skip to content

Commit fb8962b

Browse files
onewlandclaude
andauthored
feat(admin): Skip experimental analyzer for sudo-mode queries (#7696)
When validating sudo-mode queries in snuba admin, the `EXPLAIN QUERY TREE` command no longer appends `SETTINGS allow_experimental_analyzer = 1`. The SETTINGS clause can break syntax for certain sudo-mode queries, specifically CREATE/DROP WORKLOAD commands. By skipping the experimental analyzer setting for sudo-mode queries, we prevent validation failures for these commands. ## Changes - Added `sudo_mode` parameter to `is_query_using_only_system_tables()` - Added `sudo_mode` parameter to `is_valid_system_query()` - Conditionally skip the experimental analyzer setting when `sudo_mode=True` - Added test to verify the experimental analyzer is skipped for sudo queries ## Test plan - All existing tests pass: `python -m pytest tests/admin/test_system_queries.py -v` - New test added to verify experimental analyzer is not used for sudo-mode queries - No mypy errors in changed files 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 1785874 commit fb8962b

File tree

2 files changed

+63
-5
lines changed

2 files changed

+63
-5
lines changed

snuba/admin/clickhouse/system_queries.py

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -151,15 +151,15 @@ def is_query_using_only_system_tables(
151151
storage_name: str,
152152
sql_query: str,
153153
clusterless_mode: bool,
154+
sudo_mode: bool = False,
154155
) -> bool:
155156
"""
156157
Run the EXPLAIN QUERY TREE on the given sql_query and check that the only tables
157158
in the query are system tables.
158159
"""
159160
sql_query = sql_query.strip().rstrip(";") if sql_query.endswith(";") else sql_query
160-
explain_query_tree_query = (
161-
f"EXPLAIN QUERY TREE {sql_query} SETTINGS allow_experimental_analyzer = 1"
162-
)
161+
settings_clause = "" if sudo_mode else " SETTINGS allow_experimental_analyzer = 1"
162+
explain_query_tree_query = f"EXPLAIN QUERY TREE {sql_query}{settings_clause}"
163163
explain_query_tree_result = _run_sql_query_on_host(
164164
clickhouse_host,
165165
clickhouse_port,
@@ -194,6 +194,7 @@ def is_valid_system_query(
194194
storage_name: str,
195195
sql_query: str,
196196
clusterless_mode: bool,
197+
sudo_mode: bool = False,
197198
) -> bool:
198199
"""
199200
Validation based on Query Tree and AST to ensure the query is a valid select query.
@@ -209,7 +210,7 @@ def is_valid_system_query(
209210
return False
210211

211212
return is_query_using_only_system_tables(
212-
clickhouse_host, clickhouse_port, storage_name, sql_query, clusterless_mode
213+
clickhouse_host, clickhouse_port, storage_name, sql_query, clusterless_mode, sudo_mode
213214
)
214215

215216

@@ -280,7 +281,12 @@ def validate_query(
280281
return
281282

282283
if is_valid_system_query(
283-
clickhouse_host, clickhouse_port, storage_name, system_query_sql, clusterless_mode
284+
clickhouse_host,
285+
clickhouse_port,
286+
storage_name,
287+
system_query_sql,
288+
clusterless_mode,
289+
sudo_mode,
284290
):
285291
if sudo_mode:
286292
raise InvalidCustomQuery("Query is valid but sudo is not allowed")

tests/admin/test_system_queries.py

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,3 +215,55 @@ def run_query() -> None:
215215
run_query()
216216
else:
217217
run_query()
218+
219+
220+
@pytest.mark.parametrize(
221+
"sql_query, sudo_mode",
222+
[
223+
("SELECT * FROM system.clusters;", True),
224+
("SELECT * FROM system.clusters;", False),
225+
],
226+
)
227+
@pytest.mark.clickhouse_db
228+
def test_sudo_mode_skips_experimental_analyzer(sql_query: str, sudo_mode: bool) -> None:
229+
"""
230+
Test that when sudo_mode=True, the experimental analyzer setting is not
231+
appended to the EXPLAIN QUERY TREE command.
232+
"""
233+
from unittest.mock import patch
234+
235+
with patch("snuba.admin.clickhouse.system_queries._run_sql_query_on_host") as mock_run:
236+
# Mock the response to simulate successful validation
237+
mock_result = type("MockResult", (), {"results": []})()
238+
mock_run.return_value = mock_result
239+
240+
try:
241+
is_valid_system_query(
242+
settings.CLUSTERS[0]["host"],
243+
int(settings.CLUSTERS[0]["port"]),
244+
"errors",
245+
sql_query,
246+
False,
247+
sudo_mode,
248+
)
249+
except Exception:
250+
pass # We don't care if validation fails, we just want to check the query
251+
252+
# Check that the EXPLAIN QUERY TREE was called
253+
calls = [call for call in mock_run.call_args_list if "EXPLAIN QUERY TREE" in str(call)]
254+
assert len(calls) > 0, "Expected EXPLAIN QUERY TREE to be called"
255+
256+
# Get the explain query from the call - it's the 4th positional argument (index 3)
257+
# call signature: _run_sql_query_on_host(host, port, storage, sql, sudo, clusterless)
258+
explain_query = calls[0][0][3] # Fourth argument is the SQL query
259+
260+
if sudo_mode:
261+
# Should NOT contain the experimental analyzer setting
262+
assert "allow_experimental_analyzer" not in explain_query, (
263+
f"Sudo mode should not use experimental analyzer, but got: {explain_query}"
264+
)
265+
else:
266+
# Should contain the experimental analyzer setting
267+
assert "allow_experimental_analyzer" in explain_query, (
268+
f"Non-sudo mode should use experimental analyzer, but got: {explain_query}"
269+
)

0 commit comments

Comments
 (0)