Skip to content

Fix query result cache for PromQL queries#110887

Merged
nikitamikhaylov merged 3 commits into
masterfrom
fix-promql-query-cache
Jul 18, 2026
Merged

Fix query result cache for PromQL queries#110887
nikitamikhaylov merged 3 commits into
masterfrom
fix-promql-query-cache

Conversation

@nikitamikhaylov

@nikitamikhaylov nikitamikhaylov commented Jul 17, 2026

Copy link
Copy Markdown
Member

The query result cache did not work correctly for PromQL queries, in both directions.

The promql dialect transpiles a query into SELECT * FROM prometheusQuery('table', '<promql>', now()), but ParserPrometheusQuery did not link the table expression into the children of ASTTablesInSelectQueryElement. In-place AST visitors traverse children, so astContainsNonDeterministicFunctions never saw now() and the default query_cache_nondeterministic_function_handling = 'throw' protection was silently bypassed.

The Prometheus HTTP API (/api/v1/query, /api/v1/query_range) could never populate the cache.

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix the query result cache for PromQL queries. The promql dialect bypassed the non-deterministic-function check and could serve stale results anchored at now(); the Prometheus HTTP API (/api/v1/query, /api/v1/query_range) never stored cache entries at all.

🤖 Generated with Claude Code

nikitamikhaylov and others added 2 commits July 17, 2026 17:33
`ParserPrometheusQuery` set `table_expression` of `ASTTablesInSelectQueryElement`
without linking it into `children`, so in-place AST visitors never saw the
transpiled `prometheusQuery` table function. In particular,
`astContainsNonDeterministicFunctions` missed `now()` (the 'auto' evaluation time),
so with `use_query_cache = 1` the default
`query_cache_nondeterministic_function_handling = 'throw'` protection was silently
bypassed and stale `now()`-anchored results were served from the cache for up to
the entry TTL. The cache key itself was unaffected because it is computed on a
clone of the AST, and `clone` rebuilds `children` from the typed members.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`PrometheusHTTPProtocolAPI::executePromQLQuery` destroyed the
`PullingPipelineExecutor` (whose destructor cancels all pipeline processors even
after normal completion) before `finishExecutedQuery`, so
`StreamInQueryResultCacheTransform::finalizeWriteInQueryResultCache` saw
`isCancelled()` and silently dropped every pending cache write - the query cache
could never be populated through `/api/v1/query` and `/api/v1/query_range`.

Store the buffered result while the executor is still alive, as already done for
scalar subqueries and prepared sets. This also commits the entry before the HTTP
response is finalized, so a client observing the response completion also observes
the cache entry.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [d8bef86]

Summary:

job_name test_name status info comment
Stateless tests (amd_tsan, parallel) FAIL
04408_array_fold_cancellation FAIL cidb IGNORED
Stress test (arm_tsan) FAIL
Logical error: Input nodes size mismatch in dag: A, expected: [B], [C] (STID: 4752-5ad8) FAIL cidb IGNORED

AI Review

Summary

This PR fixes two real query-cache regressions in PromQL handling, and I did not find a remaining correctness issue in the current code. The promql dialect parser now links the synthesized table expression into ASTTablesInSelectQueryElement.children, so in-place AST visitors such as astContainsNonDeterministicFunctions see the transpiled prometheusQuery call. The Prometheus HTTP API now finalizes buffered query-cache writes before PullingPipelineExecutor is destroyed, and the later generic finalization pass is safe because QueryResultCacheWriter::finalizeWrite is already idempotent. The added stateless and integration tests match the two behaviors described in the PR body.

Missing context / blind spots
  • ⚠️ I validated this by reading the current diff and adjacent code, plus the current Praktika summary for PR #110887; I did not run the new Prometheus tests locally because this checkout does not include a build directory or ready test environment.
Final Verdict

✅ No new findings.

@clickhouse-gh clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Jul 17, 2026
The style check requires queries to `system.query_log` to filter by
`current_database = currentDatabase()`.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@nikitamikhaylov
nikitamikhaylov requested a review from vitlibar July 17, 2026 15:53
@nikitamikhaylov nikitamikhaylov added the comp-promql PromQL / time-series subsystem: TimeSeries storage engine, PromQL parser, PromQL-to-SQL converter... label Jul 17, 2026
@vitlibar vitlibar self-assigned this Jul 17, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 86.10% 86.10% +0.00%
Functions 91.80% 91.80% +0.00%
Branches 78.20% 78.20% +0.00%

Changed lines: Changed C/C++ lines covered: 9/9 (100.00%) · Uncovered code

Full report · Diff report

@nikitamikhaylov

Copy link
Copy Markdown
Member Author

@groeneai help investigate the test failures.

@groeneai

Copy link
Copy Markdown
Contributor

Both red checks are unrelated to the PromQL parser/API changes here (the diff touches only ParserPrometheusQuery.cpp, PrometheusHTTPProtocolAPI.cpp, and the two PromQL tests), and neither failing test exercises PromQL.

1. 04408_array_fold_cancellation (Stateless amd_tsan, parallel) - pre-existing flaky.
The job's own randomized-settings rerun passed 6/6 ("not reproducible, likely transient"). 30-day CIDB: 60 FAIL vs 36,777 OK (~0.16%) across 15 unrelated PRs, 0 on master. arrayFold cancellation, no PromQL involved.

2. Logical error: Input nodes size mismatch in dag STID 4752-5ad8 (Stress test arm_tsan) - pre-existing trunk logical error.
It is a JOIN/ActionsDAG mismatch (__table3.number / __table2.dummy), not PromQL. 60-day CIDB: 33 distinct PRs and 3 master runs (earliest 2026-05-29), spread across Stress, AST fuzzer, and serverfuzz jobs - a randomized-workload trunk issue, not introduced here. Same message was tracked in #89066 (closed, though it still recurs on master per the above). Example master occurrence: report.

Neither failure is caused by this PR; a re-run should clear both.

@nikitamikhaylov

Copy link
Copy Markdown
Member Author

@groeneai investigate all the tests separately and submit a PR for each.

@nikitamikhaylov
nikitamikhaylov added this pull request to the merge queue Jul 18, 2026
@groeneai

Copy link
Copy Markdown
Contributor

Acknowledged. I'll investigate 04408_array_fold_cancellation and the "Input nodes size mismatch in dag" logical error separately and follow up here with a fixing PR for each (or the existing fixing-PR link where one already covers the failure).

Merged via the queue into master with commit e7287eb Jul 18, 2026
175 of 178 checks passed
@nikitamikhaylov
nikitamikhaylov deleted the fix-promql-query-cache branch July 18, 2026 13:58
@robot-clickhouse robot-clickhouse added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 18, 2026
@groeneai

Copy link
Copy Markdown
Contributor

@nikitamikhaylov done, one PR per failure:

1. 04408_array_fold_cancellation (Stateless amd_tsan, parallel) -> #110942

Pre-existing flaky, unrelated to this PR: 60 FAIL / 36,777 OK (~0.16%) over 30 days across 15 unrelated PRs, 0 on master; the job's own same-settings rerun passes 6/6. Always the same diff line, the const throw case.

Root cause: arrayFold folds a block in one executeImpl(); the in-function time check fires per fold slice, but the setup before it (selector build + scatter) is uninterruptible for this input (the selector loop checks time only every 0xFFFFF elements, and range(120000) over numbers(4) is below that), and the const array replicated across the 4 rows makes that setup ~0.88s, rivaling max_execution_time=1s. When the outer timeout wrapper kills the client before the check is reached, the grep is empty and the line prints const throw: no timeout; under the sanitizer's slowdown the heavier const setup is the one that crosses the boundary. #110942 shortens the array to range(60000) (setup ~0.51s, fold still ~7.6s >> 1s, peak <20 MiB), so the query reaches the interruptible fold quickly; reference output unchanged, 50/50 local runs pass.

Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=110887&sha=d8bef86498d329f12354b6794e5f2dd18423fd7e&name_0=PR&name_1=Stateless%20tests%20%28amd_tsan%2C%20parallel%29

2. "Input nodes size mismatch in dag" LOGICAL_ERROR (Stress test arm_tsan, STID 4752-5ad8) -> #110717

Pre-existing chronic trunk error, unrelated to this PR (no PromQL/parser code on the path). The instance here is the duplicate-qualified-input variant (__table3.number twice), which #110717 already fixes with a joinGraphHasDuplicateInputNames guard in the join order optimizer. That PR is open and green; rather than open a second PR for the same variant, it is the owner for this failure. A rerun should clear both checks.

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

Labels

comp-promql PromQL / time-series subsystem: TimeSeries storage engine, PromQL parser, PromQL-to-SQL converter... pr-improvement Pull request with some product improvements pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants