feat(query-pipeline): Add configs to customize maximum size of a query #7546
+97
−8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Comment on lines
228
to
230
| if ( | ||
| not cluster_name or cluster_name not in disable_max_query_size_check_for_clusters | ||
| ) and query_size_bytes > _max_query_size_bytes(): |
There was a problem hiding this comment.
Bug: Substring matching used instead of list membership for disable_max_query_size_check_for_clusters config.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The in operator on line 229 performs Python substring matching instead of checking for membership in a list of cluster names. If disable_max_query_size_check_for_clusters is configured as a comma-separated string (e.g., "test_cluster,prod_cluster"), a cluster name like "test" will incorrectly match as a substring, leading to the max query size check being disabled when it should not be. This bypasses a critical safety mechanism.
💡 Suggested Fix
Split the disable_max_query_size_check_for_clusters string by comma into a list of cluster names, then check for cluster_name membership in that list.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: snuba/pipeline/stages/query_execution.py#L228-L230
Potential issue: The `in` operator on line 229 performs Python substring matching
instead of checking for membership in a list of cluster names. If
`disable_max_query_size_check_for_clusters` is configured as a comma-separated string
(e.g., "test_cluster,prod_cluster"), a cluster name like "test" will incorrectly match
as a substring, leading to the max query size check being disabled when it should not
be. This bypasses a critical safety mechanism.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2826178
Contributor
There was a problem hiding this comment.
I think this change should take a couple minutes and is worth making
Comment on lines
+166
to
+170
| def _max_query_size_bytes() -> int: | ||
| return ( | ||
| state.get_int_config(MAX_QUERY_SIZE_BYTES_CONFIG, MAX_QUERY_SIZE_BYTES) | ||
| or MAX_QUERY_SIZE_BYTES | ||
| ) |
There was a problem hiding this comment.
Bug: _max_query_size_bytes() incorrectly replaces 0 config value with default due to falsy evaluation.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
In _max_query_size_bytes(), when MAX_QUERY_SIZE_BYTES_CONFIG is explicitly set to 0, state.get_int_config() correctly returns 0. However, the subsequent or MAX_QUERY_SIZE_BYTES clause evaluates 0 as falsy, causing the default MAX_QUERY_SIZE_BYTES to be used instead of the configured 0. This prevents 0 from being a valid configuration value, limiting flexibility.
💡 Suggested Fix
Replace the or operator with an explicit is not None check to correctly handle 0 as a valid configuration value. For example, return config_value if config_value is not None else MAX_QUERY_SIZE_BYTES.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: snuba/pipeline/stages/query_execution.py#L166-L170
Potential issue: In `_max_query_size_bytes()`, when `MAX_QUERY_SIZE_BYTES_CONFIG` is
explicitly set to `0`, `state.get_int_config()` correctly returns `0`. However, the
subsequent `or MAX_QUERY_SIZE_BYTES` clause evaluates `0` as falsy, causing the default
`MAX_QUERY_SIZE_BYTES` to be used instead of the configured `0`. This prevents `0` from
being a valid configuration value, limiting flexibility.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2826178
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
onewland
reviewed
Nov 20, 2025
Contributor
onewland
left a comment
onewland
left a comment
There was a problem hiding this comment.
mostly fine but I agree with the AI that you should have some delimiter for the clusters where size checking is disabled and look for exact matches
Comment on lines
228
to
230
| if ( | ||
| not cluster_name or cluster_name not in disable_max_query_size_check_for_clusters | ||
| ) and query_size_bytes > _max_query_size_bytes(): |
Contributor
There was a problem hiding this comment.
I think this change should take a couple minutes and is worth making
onewland
approved these changes
Nov 20, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have instances where we want to allow for a bigger query size. This will add 2 settings to customize if we enforce query size for a given cluster or not:
ExecutionStage.disable_max_query_size_check_for_clusters: disable checking for max query size, leaving it to ClickHouse to reject the queryExecutionStage.max_query_size_bytes: customize the global query size limit Snuba enforces