perf(query): enforce limit parameter and reduce compact row parsing allocations - #72
Merged
ZhuchkaTriplesix merged 1 commit intoSep 29, 2026
Conversation
…llocations. Closes #47 QueryParams.limit was deserialized but never used, so db.query with a limit still fully parsed and materialized unbounded result sets in memory (every cell heap-allocated as a serde_json::Value), risking OOM under ClickHouse Sandbox's 256 MB ceiling on large tables. - parse_compact_output now takes an Option<usize> limit and stops parsing (and allocating) further data rows once it's reached, instead of parsing everything and discarding the excess. handle_query passes QueryParams.limit through on both the mock and real ClickHouse paths. - Replace the Vec<&str> line collection with direct iteration over output_text.lines(), removing an intermediate allocation of the whole line list before any parsing starts. - Add query_id directly to QueryResult instead of round-tripping through serde_json::to_value(...) once to get a Value, then mutating it via as_object_mut() to splice in a queryId key.
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
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.
Summary
QueryParams.limit(sent by Querya Desktop asAppSettings.sqlResultMaxRows, typically 1,000–5,000) was deserialized insrc/rpc/handlers/query.rsbut never used anywhere.db.queryalways parsed and materialized the entire result set, which risks OOM on the 256 MB ClickHouse Sandbox ceiling for large tables.parse_compact_output(src/mapper/row_compact.rs) now takes anOption<usize> limitand stops parsing/allocating further data rows once it's reached, instead of parsing everything and discarding the excess.handle_querypassesQueryParams.limitthrough on both the mock and real ClickHouse paths.Vec<&str>line collection with direct iteration overoutput_text.lines(), dropping the intermediate allocation of the whole line list before any parsing starts.query_iddirectly toQueryResultinstead of round-tripping throughserde_json::to_value(...)once just to mutate the resultingValueviaas_object_mut()to splice in aqueryIdkey.All other
parse_compact_outputcall sites (schema introspection inschema.rs, tree building intree.rs) passNone— they read small, bounded system tables and don't need truncation.Closes #47
Test plan
cargo test— 98/98 passing, including new regression tests:parse_compact_outputlimit enforcement (under/over/zero/no limit) and itsquery_idfield, plus an end-to-endhandle_querymock test asserting alimit: 1request actually returns 1 rowcargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warnings