refactor: add typed query performance contracts#8553
Conversation
Signed-off-by: discord9 <discord9@163.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the query performance fixture tool (query_perf_fixture) by moving its core logic into a reusable library module (query_perf) under the dev-tools feature flag. It introduces robust validation, normalization, and manifest-sealing logic for performance cases, while updating the test cases and the regression runner to explicitly support database-specific queries. The review feedback highlights two key improvements: replacing the is_multiple_of method with a standard modulo operator in prom_remote_write.rs to avoid compilation errors due to missing trait imports, and using a safe .get() fallback for the database key in query_regression_runner.py to prevent potential KeyError exceptions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| args.value_base + (series_idx % 97) as f64 + effective as f64 * args.value_step | ||
| } | ||
| ValuePattern::MixedSignalRepeated => { | ||
| if local.is_multiple_of(args.value_mixed_every) { |
There was a problem hiding this comment.
Using local.is_multiple_of(args.value_mixed_every) requires the num_integer::Integer trait to be in scope, which is not imported in this file and could lead to compilation errors. It is safer and more idiomatic to use the standard modulo operator local % args.value_mixed_every == 0 instead.
| if local.is_multiple_of(args.value_mixed_every) { | |
| if local % args.value_mixed_every == 0 { |
| first_query = queries[0] | ||
| result = http_post_sql(target.http_port, first_query["query"], first_query["database"], http_timeout) | ||
| if not result["ok"]: | ||
| validation_errors.append({"sql": first_query["query"], "error": result.get("error"), "response": result.get("response")}) | ||
| validations.append(result) | ||
| measurements = [] | ||
| for q in queries: | ||
| for _ in range(int(q.get("warmup", 0))): | ||
| warmup = http_post_sql(target.http_port, q["query"], db, http_timeout) | ||
| warmup = http_post_sql(target.http_port, q["query"], q["database"], http_timeout) | ||
| if not warmup["ok"]: | ||
| validation_errors.append({"sql": q["query"], "phase": "warmup", "error": warmup.get("error"), "response": warmup.get("response")}) | ||
| samples = [] | ||
| for _ in range(int(q.get("iterations", 1))): | ||
| result = http_post_sql(target.http_port, q["query"], db, http_timeout) | ||
| result = http_post_sql(target.http_port, q["query"], q["database"], http_timeout) |
There was a problem hiding this comment.
To prevent potential KeyError exceptions if the database key is missing in any of the query definitions (especially during migration or when running custom external case TOMLs), it is safer to use .get("database", "public") with a fallback default value.
| first_query = queries[0] | |
| result = http_post_sql(target.http_port, first_query["query"], first_query["database"], http_timeout) | |
| if not result["ok"]: | |
| validation_errors.append({"sql": first_query["query"], "error": result.get("error"), "response": result.get("response")}) | |
| validations.append(result) | |
| measurements = [] | |
| for q in queries: | |
| for _ in range(int(q.get("warmup", 0))): | |
| warmup = http_post_sql(target.http_port, q["query"], db, http_timeout) | |
| warmup = http_post_sql(target.http_port, q["query"], q["database"], http_timeout) | |
| if not warmup["ok"]: | |
| validation_errors.append({"sql": q["query"], "phase": "warmup", "error": warmup.get("error"), "response": warmup.get("response")}) | |
| samples = [] | |
| for _ in range(int(q.get("iterations", 1))): | |
| result = http_post_sql(target.http_port, q["query"], db, http_timeout) | |
| result = http_post_sql(target.http_port, q["query"], q["database"], http_timeout) | |
| first_query = queries[0] | |
| result = http_post_sql(target.http_port, first_query["query"], first_query.get("database", "public"), http_timeout) | |
| if not result["ok"]: | |
| validation_errors.append({"sql": first_query["query"], "error": result.get("error"), "response": result.get("response")}) | |
| validations.append(result) | |
| measurements = [] | |
| for q in queries: | |
| for _ in range(int(q.get("warmup", 0))): | |
| warmup = http_post_sql(target.http_port, q["query"], q.get("database", "public"), http_timeout) | |
| if not warmup["ok"]: | |
| validation_errors.append({"sql": q["query"], "phase": "warmup", "error": warmup.get("error"), "response": warmup.get("response")}) | |
| samples = [] | |
| for _ in range(int(q.get("iterations", 1))): | |
| result = http_post_sql(target.http_port, q["query"], q.get("database", "public"), http_timeout) |
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
None.
What's changed and what's your intention?
This draft starts the independent Rust migration of the query performance regression tooling. It establishes typed, validated contracts before replacing the endpoint runner and local lifecycle controller.
dev-tools-gatedcmd::query_perfmodule.count_allfallback so missing query definitions fail closed instead of silently measuring a different workload.This is intentionally a Draft PR. Follow-up commits in this PR will add the endpoint-only Rust measure/finalize runner and an explicit local/ARC controller, then remove the Python runner. Kubernetes control-plane support remains out of scope.
Compatibility: production behavior and public APIs are unchanged because the Rust tooling and optional dependencies are gated behind the non-default
dev-toolsfeature.Validation completed for this checkpoint:
make fmtmake fmt-tomlcargo check -p cmd --features dev-tools --lib --tests --bin query_perf_fixturecargo check -p cmd --bin greptimeThe focused test targets compile, but full local test-binary linking is currently blocked by repeated SIGTERM termination in this environment; CI remains the execution gate.
PR Checklist