Skip to content

Fix deriv to scale its slope by the timestamp tick resolution - #112360

Merged
nikitamikhaylov merged 6 commits into
ClickHouse:masterfrom
valerypetrov:promql/fix-deriv-scale
Aug 6, 2026
Merged

Fix deriv to scale its slope by the timestamp tick resolution#112360
nikitamikhaylov merged 6 commits into
ClickHouse:masterfrom
valerypetrov:promql/fix-deriv-scale

Conversation

@valerypetrov

@valerypetrov valerypetrov commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Related: #57545

Changelog category (leave one):

  • Experimental Feature

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

PromQL: implemented function deriv, fixing a bug where its backing aggregate function returned values 1000x too small with millisecond-resolution timestamps.

Description

deriv(v range-vector) was rejected with Function 'deriv' is not implemented, even though its backing aggregate function (timeSeriesDerivToGrid) already existed, registered and dispatched by predict_linear today. Wiring it in as-is would not have produced 6 passes — it would have silently produced values 1000x too small.

Root cause: AggregateFunctionTimeseriesLinearRegressionTraits::Aggregator::getResult() returns the raw OLS slope for the deriv (!is_predict) branch without multiplying by Base::timestamp_scale_multiplier, unlike AggregateFunctionTimeseriesExtrapolatedValueTraits (backing rate/increase), which does this exact conversion. Since ClickHouse's TimeSeries engine stores timestamps as DateTime64(3) (millisecond ticks) by default, and Prometheus's own linearRegression() computes the slope against sample timestamps converted to seconds, the unscaled result was off by exactly 1000x. Independent corroboration: the existing stateless test (03573_linear_regression_timeseries_functions.sql) fed toUnixTimestamp(timestamp) (seconds) into timeSeriesDerivToGrid while feeding the raw DateTime64(3) column directly into timeSeriesPredictLinearToGrid in the same query — a manual workaround at the call site rather than a fix.

Fix: thread timestamp_scale_multiplier into the Aggregator the same way createAggregator() already threads start_timestamp, and multiply only the !is_predict return value by it. predict_linear's is_predict branch (already scale-correct by construction, since its predict-offset parameter is pre-converted to the column's native tick scale) is untouched and verified to produce bit-identical output to before this change.

Also removed the toUnixTimestamp() workaround from 03573_linear_regression_timeseries_functions.sql now that deriv is scale-correct on its own, and updated its .reference (one value shifts by a single ULP from computing in millisecond- vs second-scale coordinates; predict_linear's expected value in the same test is bit-identical to before).

Wired deriv into applyFunctionOverRange.cpp's impl_map with drop_metric_name = true, matching rate/increase/delta's convention (predict_linear remains unwired — separate, larger task, since its 2-argument shape doesn't fit the existing 1-argument dispatcher).

Verified against Prometheus 3.5.0.

Testing: updated the existing SQL-level test to drop its scaling workaround, plus a new case in tests/integration/test_prometheus_protocols/test_evaluation.py covering deriv() over a plain range-vector selector with independently-derived expected values. This flips 6 queries of the PromQL compliance suite from unsupported to passed, with zero regressions.

Version info

  • Merged into: 26.8.1.923 (included in 26.8 and later)

AggregateFunctionTimeseriesLinearRegressionTraits::Aggregator::getResult()
returned the raw OLS slope for timeSeriesDerivToGrid (deriv) without
multiplying by Base::timestamp_scale_multiplier, unlike
AggregateFunctionTimeseriesExtrapolatedValueTraits (rate/increase), which
does. Since the TimeSeries engine stores timestamps as DateTime64(3)
(millisecond ticks) by default, deriv() returned values 1000x too small
versus real Prometheus, whose linearRegression() converts sample
timestamps to seconds before computing the slope.

Thread timestamp_scale_multiplier into the Aggregator the same way
createAggregator() already threads start_timestamp, and multiply only the
!is_predict (deriv) result by it; predict_linear's is_predict branch is
untouched and produces bit-identical output (verified against the
existing stateless test).

Also drop the toUnixTimestamp() workaround from
03573_linear_regression_timeseries_functions.sql now that deriv is
scale-correct on its own, and update the .reference file (one value
shifts by a single ULP from computing in millisecond- instead of
second-scale coordinates).

Wire deriv into PromQL by adding it to applyFunctionOverRange.cpp's
impl_map (timeSeriesDerivToGrid, drop_metric_name = true, matching
rate/increase/delta's convention), and add a PromQL-level regression test
to test_evaluation.py.

Verified: all 6 deriv(demo_disk_usage_bytes[1s/15s/1m/5m/15m/1h]) compliance
queries move from unsupported to pass, with zero regressions (467 pass /
1 fail / 69 unsupported / 2 ref_mismatch, same pre-existing fail/mismatch
as before this change).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Jul 29, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [75137b6]

Summary:

job_name test_name status info comment
Integration tests (amd_msan, 4/8) FAIL
test_tcp_handler_connection_limits/test.py::test_query_count_limit FAIL cidb, issue

AI Review

Summary

This PR fixes timeSeriesDerivToGrid so DateTime64 timestamps are converted back to per-second slopes, wires deriv into the PromQL range-function dispatcher, and updates the table-function source documentation plus regression coverage. After checking the current diff, the surrounding time-series implementation, and the prior review threads, I did not find a remaining correctness, compatibility, or documentation-source issue in the current head.

Final Verdict

✅ No blocking or major findings.

LLVM Coverage Report

Metric Baseline Current Δ
Lines 86.50% 86.50% +0.00%
Functions 91.90% 91.90% +0.00%
Branches 78.70% 78.80% +0.10%

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

Full report · Diff report

@clickhouse-gh clickhouse-gh Bot added the pr-experimental Experimental Feature label Jul 29, 2026
@valerypetrov
valerypetrov force-pushed the promql/fix-deriv-scale branch from fa64ab1 to b356cac Compare July 29, 2026 00:36
@alexey-milovidov alexey-milovidov changed the title Fix deriv() to scale its slope by the timestamp tick resolution Fix deriv to scale its slope by the timestamp tick resolution Jul 29, 2026
# Conflicts:
#	src/Storages/TimeSeries/PrometheusQueryToSQL/applyFunctionOverRange.cpp
#	tests/integration/test_prometheus_protocols/test_evaluation.py
@clickhouse-gh

clickhouse-gh Bot commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 86.50% 86.50% +0.00%
Functions 91.90% 91.90% +0.00%
Branches 78.70% 78.70% +0.00%

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

Full report · Diff report

@valerypetrov
valerypetrov requested a review from a team as a code owner August 3, 2026 16:53
@nikitamikhaylov nikitamikhaylov added the comp-promql PromQL / time-series subsystem: TimeSeries storage engine, PromQL parser, PromQL-to-SQL converter... label Aug 3, 2026
@nikitamikhaylov nikitamikhaylov self-assigned this Aug 3, 2026
@nikitamikhaylov

Copy link
Copy Markdown
Member

@groeneai make sure all the Stress tests failures addressed in separate PRs.

@groeneai

groeneai commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Stress test failures on 13b616e6: owners

All five red checks are Stress test jobs, and none of them is caused by this PR: it only touches
applyFunctionOverRange.cpp, while three of the five signatures also fire on master and across
57 to 164 unrelated PRs in the last 30 days. One row per red check, with its owner:

Check Signature (STID) Breadth, 30d Owner
Stress test (arm_tsan) ColumnBLOB should be converted to a regular column before usage (3059-3284) 87 rows / 57 PRs / 7 master #111997 (mine, open)
Stress test (amd_tsan) same (3059-3284) as above #111997
Stress test (arm_debug) same (3059-3663) as above #111997
Stress test (arm_asan_ubsan, s3) Not-ready Set is passed as the second argument for function 'globalNotNullIn' (0250-4e52) 212 rows / 164 PRs / 10 master #102308 (open)
Stress test (amd_msan) Coordinator for parallel reading from replicas is not initialized (6383-2785) 2 rows / 2 PRs / 0 master no fixing PR exists: a fix task is created, and I will post the fixing-PR link in this thread

ColumnBLOB. Both aborts are columnMatchesType (validateColumnType.cpp:70) reached from
ExpressionActions::executeAction. The arm_tsan trigger is
SELECT item_id FROM merge(REGEXP('.+'), 't1') GROUP BY item_id; the arm_debug one is
SELECT _table FROM m2 where m2 = Merge('^(d1|d4)$') and d4 is Distributed. Both are the
Merge-spanning-a-distributed-table shape of #111945, which #111997 fixes by not adding
BlocksMarshallingStep to plans whose blocks are consumed in the same process.

Not-ready Set. Thrown at in.cpp:116 under FilterTransform. The query is a stress mutation of
01231_operator_null_in_2.sql with transform_null_in = 1:
... WHERE i GLOBAL NOT IN (SELECT i FROM cluster('test_cluster_with_incorrect_pw', currentDatabase(), 'null_in_subquery') WHERE dt = 1).
GLOBAL puts this on the set_and_key->external_table branch of
FutureSetFromSubquery::buildOrderedSetInplace (PreparedSets.cpp:499), which merged #107924
deliberately left on the destructive in-place build; #102308 removes that exclusion. #102192 is not
the owner here, it does not touch PreparedSets.cpp. One thing possibly useful for #102308: the
mutated cluster has a deliberately wrong password, so this looks like a trigger that does not need
the prepared_sets_build_ordered_set_inplace_fail failpoint, which has been the only handle on that
branch so far.

Coordinator for parallel reading. New signature, 2 hits ever, both today, the other on #102192,
0 on master. Thrown at RemoteQueryExecutor.cpp:877. Two followers of one initial query
(0d68b853-1fdb-4fe8-ad85-8bc8339afe2a) abort in the same millisecond on
03232_pr_not_ready_set.sql: the outer query runs with max_parallel_replicas = 3, and its IN
subquery reads through a cluster() table function, so each follower starts a nested distributed
read whose executor has no parallel_reading_coordinator and then receives a
MergeTreeAllRangesAnnouncement. I am investigating this one and will send a separate PR.

Session id: cron:clickhouse-premium-slot-25:20260803-233900

…scale

Resolves conflicts from master's docs migration off of docs/en/**/*.md
to autogenerated Mintlify docs/reference/**/*.mdx:

- docs/en/sql-reference/table-functions/prometheusQuery.md and
  prometheusQueryRange.md were deleted upstream in favor of the new
  autogenerated pages. Removed them and ported this PR's addition of
  `deriv` to the Range functions list into the true source of that
  content, src/TableFunctions/TableFunctionTimeSeries.cpp (both the
  prometheusQuery and prometheusQueryRange doc strings), and mirrored
  the same change into the corresponding generated
  docs/reference/functions/table-functions/prometheusQuery.mdx and
  prometheusQueryRange.mdx AUTOGENERATED_START/END regions so the
  committed docs stay in sync with source.

src/Storages/TimeSeries/PrometheusQueryToSQL/applyFunctionOverRange.cpp
auto-merged cleanly: this PR added the `deriv` -> `timeSeriesDerivToGrid`
dispatch entry while master independently renamed `PQT::Function` to
`PrometheusQueryTree::Function` in an unrelated part of the same file;
both changes coexist correctly in the merge result.

No other files under src/AggregateFunctions/TimeSeries,
src/AggregateFunctions/examples, or the
03573_linear_regression_timeseries_functions test files were touched by
master since this branch's last sync, so the deriv scaling fix itself
carried through unmodified.
@groeneai

groeneai commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

The fixing PR for the Stress test (amd_msan) row of the ledger above (Coordinator for parallel reading from replicas is not initialized, STID 6383-2785) is now open: #113440. It closes #113310.

Root cause: RemoteQueryExecutor::sendQuery copied the current ClientInfo and only ever forced collaborate_with_initiator on, never off, so a follower's true leaked into a nested distributed read whose executor deliberately carries no coordinator. The next hop then acted as a follower of a coordinator absent on that connection and announced into the coordinator-less executor. The flag is now derived from that executor's own extension.

Nothing else is owed on this PR from that row.

Comment thread docs/reference/functions/table-functions/prometheusQuery.mdx Outdated
@nikitamikhaylov nikitamikhaylov added the pr-autogenerated-docs PR that regenerates docs artifacts from source; exempt from the autogenerated-region edit guard label Aug 5, 2026
The .mdx files are generated from TableFunctionTimeSeries.cpp, which
already carries the deriv addition.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@nikitamikhaylov nikitamikhaylov removed the pr-autogenerated-docs PR that regenerates docs artifacts from source; exempt from the autogenerated-region edit guard label Aug 5, 2026
@nikitamikhaylov
nikitamikhaylov added this pull request to the merge queue Aug 6, 2026
Merged via the queue into ClickHouse:master with commit 16bd8e4 Aug 6, 2026
178 of 180 checks passed
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Aug 6, 2026
nikitamikhaylov added a commit that referenced this pull request Aug 6, 2026
…e for #112360

The merged deriv scale fix changes timeSeriesDerivToGrid output from per-tick
to per-second on DateTime64(3) timestamps (x1000); one deriv line in the
reference updates accordingly, all other outputs are unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
nikitamikhaylov added a commit that referenced this pull request Aug 6, 2026
…e for #112360

The merged deriv scale fix changes timeSeriesDerivToGrid output from per-tick
to per-second on DateTime64(3) timestamps (x1000); one deriv line in the
reference updates accordingly, all other outputs are unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
nikitamikhaylov added a commit that referenced this pull request Aug 7, 2026
…e for #112360

The merged deriv scale fix changes timeSeriesDerivToGrid output from per-tick
to per-second on DateTime64(3) timestamps (x1000); one deriv line in the
reference updates accordingly, all other outputs are unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors comp-promql PromQL / time-series subsystem: TimeSeries storage engine, PromQL parser, PromQL-to-SQL converter... manual approve Manual approve required to run CI pr-experimental Experimental Feature 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.

5 participants