Skip to content

refactor: add typed query performance contracts#8553

Draft
discord9 wants to merge 1 commit into
GreptimeTeam:mainfrom
discord9:fix/perf-runner-query-fallback
Draft

refactor: add typed query performance contracts#8553
discord9 wants to merge 1 commit into
GreptimeTeam:mainfrom
discord9:fix/perf-runner-query-fallback

Conversation

@discord9

Copy link
Copy Markdown
Contributor

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.

  • Move the query-performance case model and fixture implementations into a shared dev-tools-gated cmd::query_perf module.
  • Add strict workload validation, checked arithmetic, typed fixture/run manifests, canonical digests, and cross-manifest consistency checks.
  • Make fixture provider semantics explicit: API-write fixtures require no SST files, while externally prepared fixtures require valid files and IDs.
  • Migrate all built-in case TOMLs to the strict schema.
  • Remove the Python runner's implicit count_all fallback 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-tools feature.

Validation completed for this checkpoint:

  • make fmt
  • make fmt-toml
  • cargo check -p cmd --features dev-tools --lib --tests --bin query_perf_fixture
  • cargo check -p cmd --bin greptime
  • Parsed and validated all 10 built-in case plans
  • Exercised dry-run fixture generation for all 5 direct-SST cases

The 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

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR requires documentation updates.
  • API changes are backward compatible.
  • Schema or data changes are backward compatible.

Signed-off-by: discord9 <discord9@163.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
if local.is_multiple_of(args.value_mixed_every) {
if local % args.value_mixed_every == 0 {

Comment on lines +716 to +729
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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-required This change requires docs update. size/XXL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant