Skip to content

fix: preserve PromQL plan identity for MetricsQL - #559

Closed
zzylol wants to merge 6 commits into
mainfrom
review/pr553
Closed

zzylol wants to merge 6 commits into
mainfrom
review/pr553

Conversation

@zzylol

@zzylol zzylol commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Before this PR

Merged MetricsQL work reused the stable PromQL QueryPlanEntry identity/serde contract for non-PromQL queries, and tenant-prefixed VictoriaMetrics routes could consult a global MetricsQL catalog and SummaryStore. ClickHouse publication also needed to remain an independent sidecar while sharing the executable DAG payload.

After this PR

  • Restores the existing PromQL QueryPlanEntry { canonical_promql, ... } wire contract and exact lookup behavior.
  • Introduces a language-neutral ExecutableQueryPlan payload while keeping independent MetricsQlPlanCatalog and ClickHouse SQL plan catalogs.
  • Compiles MetricsQL into its independent canonical identity plus the shared executable DAG, without storing MetricsQL or SQL identity in PromQL fields.
  • Publishes PromQL, MetricsQL, ClickHouse SQL, SDS, precompute, and transmission views in one physical publication request and installs all validated sidecars in one immutable active generation.
  • Validates MetricsQL and ClickHouse bindings against the installed SDS/precompute definitions before staging; invalid sidecars cannot replace the active snapshot.
  • Executes PromQL, MetricsQL, and ClickHouse entries through a borrowed ExecutablePlanView; language sidecars never construct a fake QueryPlanEntry or clone the DAG. Descriptor lookup and readout consume the same borrowed payload.
  • Forces tenant-scoped VictoriaMetrics instant and range requests to exact fallback while SDS/catalog state remains global, preserving tenant headers.
  • Preserves startup artifact compatibility and updates ClickHouse/VM process fixtures to carry their independent sidecars.

Verification

  • cargo check --all-targets -q — passed for the full workspace.
  • cargo test -q -p control_plane --lib executable_extraction_preserves_promql_entry_serde — passed; asserts the exact legacy PromQL JSON shape.
  • cargo test -q -p data_plane --lib victoriametrics_instant_and_range_fail_closed_for_each_tenant — passed for tenant-a and tenant-b across instant/range guards.
  • cargo test -q -p data_plane --lib active_metricsql_entry_reaches_the_shared_dag_executor — passed.
  • cargo test -q -p data_plane --lib catalog_install_tests — 6 passed, including invalid ClickHouse sidecar atomicity and invalid MetricsQL pane binding rejection.
  • git diff --check — passed.

Final independent verification

  • Real ClickHouse exact SQL differential and Grafana smoke against clickhouse/clickhouse-server:latest: 1/1 passed.
  • Real ClickHouse reader → backfill service lifecycle: 1/1 passed with authenticated HTTP user.
  • ClickHouse module tests: 12/12 passed.
  • Real VictoriaMetrics accelerated differential: passed.
  • KLL/HLL/CMS/CountSketch process oracles: 4/4 passed.
  • PromQL query-plan regressions: 32/32 passed.
  • Full control-plane library suite: cargo test -p control_plane --lib -q — 712/712 passed on head e0511a05.
  • CandidateTopK physical compilation regression: restored exact-subquery witness lowering using the compiler-owned logical source; the focused regression and full control-plane suite pass without changing the PromQL plan wire contract.

@zzylol
zzylol marked this pull request as ready for review September 10, 2026 09:25
@zzylol

zzylol commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Closing because this branch implements independent MetricsQL/ClickHouse sidecar catalogs that conflict with the unified QueryLanguage + shared QueryPlan DAG now on main. Any remaining MetricsQL behavior must be replayed onto the unified DAG rather than merging this alternate architecture.

@zzylol zzylol closed this Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant