Skip to content

Update two-stacks thresholds of promql function deriv() - #118987

Merged
nikitamikhaylov merged 1 commit into
ClickHouse:masterfrom
vitlibar:timeseries-two-stacks-thresholds
Sep 9, 2026
Merged

Update two-stacks thresholds of promql function deriv()#118987
nikitamikhaylov merged 1 commit into
ClickHouse:masterfrom
vitlibar:timeseries-two-stacks-thresholds

Conversation

@vitlibar

@vitlibar vitlibar commented Sep 9, 2026

Copy link
Copy Markdown
Member

Changelog category (leave one):

  • Performance Improvement

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

Update two-stacks thresholds of promql function deriv()


Workflow [PR]
Sync PR [sync-upstream/pr/118987]

Version info

  • Merged into: 26.9.1.1073 (included in 26.9 and later)

@clickhouse-gh

clickhouse-gh Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [46d27f5]

Summary:


AI Review

Summary

This PR lowers the two-stack selection thresholds used by the shared linear-regression time-series aggregators and updates the stateless/performance tests so they still exercise the intended recompute, average-density, and hard-cap paths. I did not find a code-level correctness or coverage problem in the implementation, but the PR template metadata does not describe the actual change accurately.

PR Metadata
  • Changelog category is not correct for this change. This is a performance-tuning change for existing experimental functions, not a new experimental feature. Use Performance Improvement.
  • A Changelog entry is required for Performance Improvement, and the current entry is too vague and only names deriv() even though the shared linear-regression trait also changes timeSeriesPredictLinearToGrid.
  • Suggested replacement:
### Changelog category (leave one):
- Performance Improvement

### Changelog entry (a [user-readable short description](https://github.com/ClickHouse/ClickHouse/blob/master/docs/changelog_entry_guidelines.md) of the changes that goes into CHANGELOG.md):
Lowered the threshold that switches experimental `timeSeriesDerivToGrid` and `timeSeriesPredictLinearToGrid` from window recomputation to the two-stack sliding aggregator, improving finalize performance for windows with 4 to 11 populated buckets.
Final Verdict

✅ No technical blockers in the code or tests. Fix the PR metadata before merge.

LLVM Coverage Report

⚠️ No coverage measurement for commit 46d27f5: incomplete coverage measurement: 1 of 21 shard profiles are missing: LLVM_COVERAGE_FILE_it_7.profdata.

@clickhouse-gh clickhouse-gh Bot added the pr-experimental Experimental Feature label Sep 9, 2026
/// capacity we stop trusting the average and bound the worst case: at this size a fully dense window already
/// makes recompute ~2x slower than two-stacks (measured by the `timeseries_to_grid_two_stack_vs_recompute` example).
static constexpr size_t BPW_TO_FORCE_TWO_STACKS = 20;
static constexpr size_t BPW_TO_FORCE_TWO_STACKS = 12;

@vitlibar vitlibar Sep 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Recent measurements after our optimizations showed different values than what were measured originally:

AVG_POPULATED_BPW_TO_ENABLE_TWO_STACKS (two-stacks first wins) = 4
BPW_TO_FORCE_TWO_STACKS (two-stacks > 2x faster) = 12

So these thresholds in the code should be updated

<value>50</value>
<value>160</value>
<value>30</value>
<value>80</value>

@vitlibar vitlibar Sep 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Also the performance and stateless tests were adjusted to follow their claimed scenarios.

@vitlibar vitlibar added the comp-promql PromQL / time-series subsystem: TimeSeries storage engine, PromQL parser, PromQL-to-SQL converter... label Sep 9, 2026
@clickhouse-gh

clickhouse-gh Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Build profile diff (arm_release)

Comparing 46d27f54c with master a05aaa178 (stripped binary size, per-symbol sizes and ThinLTO time; compile times per translation unit against the most recent warmup build that recompiled it).

✅ No significant changes.

Binary sizes

programs/clickhouse-stripped: smaller than the master baseline by the known offset between the two builds, so the difference is not shown. A delta that differs from the offset by more than 50% of it is shown, in either direction.

The official master build is compiled with -g and a pull request build is not, and XRay counts debug instructions towards its instrumentation threshold, so master instruments thousands of functions more and its binary is ~0.4% larger no matter what the pull request does.

Compile time of recompiled translation units

8 translation units recompiled, 45 s compile time in total, 8 of them have a recent master baseline.

Job report

@nikitamikhaylov
nikitamikhaylov added this pull request to the merge queue Sep 9, 2026
Merged via the queue into ClickHouse:master with commit 3e1d3c1 Sep 9, 2026
174 checks passed
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-experimental Experimental Feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants