iframe-proxy

vitlibar · 2026-05-13T09:29:13Z

Reverts #104672, with extra improvements.
This PR is an improved version of #103428

This PR adds:
1. Preserving parameter types in timeSeries* aggregate functions

We need to restore the initial invariant: For any aggregation function IAggregateFunction::getParameters() should return exactly the parameters used to create the aggregation function. Aggregation functions are not allowed to change or normalize these parameters.

Otherwise it would become a different DataTypeAggregateFunction (see DataTypeAggregateFunction::strictEquals),
and could fail to reattach a table because decodeDataType() would reconstruct a type different from the one
recorded in the column metadata; or fail on parallel replicas under serialize_query_plan=1 because the replica's
decodeDataType() would reconstruct a type different from the one the coordinator sent in the plan. And we actually saw a bunch of such issues already (#101407, #102345, #103079)

It's difficult to fix all these issues and it's better to just restore the invariant. By the way, for non-timeseries aggregate functions parameter types are always preserved.

2. Parameters are now printed with types in timeSeries* aggregate functions.

Another important change is that for the timeSeries* aggregate functions, the AggregateFunction(...) type name now prints parameters in a type-annotated form ('<value>'::TypeName) instead of a bare literal that could be ambiguous. For example this is how a data type is printed now

AggregateFunction(timeSeriesInstantRateToGrid('1734004810'::Decimal64(3), '1734004860'::Decimal64(3), 10, 60), Array(DateTime64(3, 'UTC')), Array(Float64))

and earlier it was printed this way:

AggregateFunction(timeSeriesInstantRateToGrid('1734004810', '1734004860', 10, 60), Array(DateTime64(3, 'UTC')), Array(Float64))

Bare literals were ambiguous because '1734004810' could mean a String or a Decimal with a different scale. Form '<value>'::TypeName allows to parse exactly the same parameters back.

Closes #104459

Version info

Merged into: 26.6.1.98

…egate functions""

clickhouse-gh · 2026-05-13T09:29:52Z

Workflow [PR], commit [6e4db42]

Summary: ✅

AI Review

Summary

This PR restores parameter-type preservation for time-series aggregate functions and adds typed literal round-tripping for DataTypeAggregateFunction names. The core direction is correct, but there are still unresolved contract gaps: two parameterized aggregates still violate the preserved-parameters invariant, the new factory assertion has a global escape hatch that can hide future regressions, and the key serialize_query_plan = 1 + parallel-replicas path remains under-proven by focused coverage.

Findings

❌ Blockers

[src/AggregateFunctions/AggregateFunctionKolmogorovSmirnovTest.cpp:233, src/AggregateFunctions/AggregateFunctionMannWhitney.cpp:146] AggregateFunctionKolmogorovSmirnov and AggregateFunctionMannWhitney still pass {} to IAggregateFunctionDataHelper while behavior depends on constructor params. This still violates the stated invariant that IAggregateFunction::getParameters must return creation parameters, and keeps state-type identity dependent on special-case handling.
Suggested fix: pass params to both base constructors and remove dependency on exception handling in AggregateFunctionFactory.

⚠️ Majors

[src/AggregateFunctions/AggregateFunctionFactory.cpp:235, src/AggregateFunctions/AggregateFunctionFactory.cpp:279] The new invariant check accepts function->getParameters().empty() globally. That makes the guard silent for any future parameterized aggregate that accidentally drops parameters, which weakens the intended regression detector.
Suggested fix: narrow this to explicit temporary exceptions only, then remove those exceptions once fixed.

Tests

⚠️ [tests/queries/0_stateless/03254_timeseries_instant_value_aggregate_functions.sql:103] The section intended to validate plan/header behavior under serialize_query_plan = 1 still does not explicitly enable parallel replicas in that section (enable_parallel_replicas, max_parallel_replicas, parallel_replicas_for_non_replicated_merge_tree). Since enable_parallel_replicas defaults to 0, this does not provide focused evidence for the exact parallel-replica serialization path cited in the PR motivation.

Final Verdict

Status: ⚠️ Request changes
Minimum required actions:

Make AggregateFunctionKolmogorovSmirnov and AggregateFunctionMannWhitney preserve constructor params in getParameters.
Replace the global empty-parameter bypass in the new factory assertion with explicit temporary exceptions.
Add one deterministic stateless check that combines serialize_query_plan = 1 with explicitly enabled parallel replicas for the affected time-series path.

clickhouse-gh · 2026-05-13T09:38:38Z

+        /// and could fail to reattach a table because decodeDataType() would reconstruct a type different from the one
+        /// recorded in the column metadata; or fail on parallel replicas under serialize_query_plan=1 because the replica's
+        /// decodeDataType() would reconstruct a type different from the one the coordinator sent in the plan.)
+        /// NOTE: Check for function->getParameters().empty() is here because some aggregation functions (kolmogorovSmirnovTest, mannWhitneyUTest) drop their parameters completely.


The new invariant check is intentionally bypassed for kolmogorovSmirnovTest/mannWhitneyUTest, but function->getParameters().empty() makes that bypass global.

If any other aggregate function accidentally starts dropping parameters entirely, this assertion will stay silent and we will miss exactly the regression this guard is meant to catch.

Can we narrow the exception to an explicit allowlist (for example by function name) instead of accepting every empty-parameter result?

This is still unresolved in the current code.

AggregateFunctionFactory::getImpl still accepts function->getParameters().empty() as a global bypass. That means any future parameterized aggregate that accidentally drops parameters will silently skip the invariant check.

Could we narrow this to a temporary explicit exception list (or remove the bypass once the known offenders are fixed) so new regressions are not masked?

This still looks unresolved in the current code.

AggregateFunctionFactory::getImpl still allows a global bypass via function->getParameters().empty():

chassert(function && (function->getParameters().empty() || function->getParameters() == parameters), ...)

That makes the new invariant check silent for any parameterized aggregate that accidentally starts dropping parameters, not just the known temporary exceptions.

Can we narrow this to an explicit temporary exception list (or remove it once the remaining offenders are fixed) so the assertion keeps catching new regressions?

This still looks unresolved on the current head.

In src/AggregateFunctions/AggregateFunctionFactory.cpp, the invariant check still accepts function->getParameters().empty():

chassert(function && (function->getParameters().empty() || function->getParameters() == parameters), ...)

Because that empty-parameters branch is global, any new parameterized aggregate that accidentally drops parameters will bypass the check and the regression will stay undetected.

Could we narrow this to explicit temporary exceptions only (or remove it once the remaining offenders are fixed)?

Fixed in #106245

Copilot

Pull request overview

This PR re-applies an earlier change (#103428 / #101083) that restores the invariant that IAggregateFunction::getParameters() must return exactly the parameters used to construct the function. Previously, the timeseries *ToGrid aggregate functions normalized their first four (and the predict-offset) parameters to Decimal64, which could cause DataTypeAggregateFunction mismatches between the column metadata and the reconstructed type — breaking table reattach and serialize_query_plan=1 parallel replicas. The PR threads the original parameters Array through to the base aggregate constructor and adds chasserts in AggregateFunctionFactory::getImpl to enforce the invariant for both direct functions and combinator-wrapped functions.

Changes:

Pass the original parameters Array through the timeseries aggregate factory and base/derived constructors instead of building a normalized Decimal64 array in the base.
Add chasserts in AggregateFunctionFactory::getImpl enforcing function->getParameters() == parameters, with an explicit hardcoded exception for kolmogorovSmirnovTest/mannWhitneyUTest (which still discard params; TODO comments added at their definitions).
Add a regression test (04109_…parameter_type_mismatch) showing that mismatched parameter spellings produce CANNOT_CONVERT_TYPE (matching non-timeseries behaviour), and stabilize two existing timeseries tests by disabling randomized parallel replicas.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/AggregateFunctions/AggregateFunctionFactory.cpp	Adds invariant chasserts after creator/combinator dispatch; removes redundant `AggregateFunctionNothing` special case.
src/AggregateFunctions/IAggregateFunction.h	Comment on `getParameters()` clarifying the new invariant.
src/AggregateFunctions/AggregateFunctionMannWhitney.cpp	TODO noting parameters should be passed through (currently dropped).
src/AggregateFunctions/AggregateFunctionKolmogorovSmirnovTest.cpp	Same TODO for KS test.
src/AggregateFunctions/TimeSeries/AggregateFunctionTimeseriesBase.h	Base constructor now takes `parameters_` and forwards it instead of building a normalized array.
src/AggregateFunctions/TimeSeries/AggregateFunctionTimeseriesLinearRegression.h	Predict-variant constructor takes/forwards `parameters_`; clarifying comment.
src/AggregateFunctions/TimeSeries/AggregateFunctionTimeseriesHelpers.cpp	Factory passes the original `parameters` to all four constructor call sites.
tests/queries/0_stateless/04109_timeseries_aggregate_function_parameter_type_mismatch.{sql,reference}	New regression test for the restored invariant (CANNOT_CONVERT_TYPE on mismatched spelling).
tests/queries/0_stateless/03254_timeseries_instant_value_aggregate_functions.{sql,reference}	Disable randomized PR; reference now reflects un-normalized parameter spellings (e.g. `1734004810.1234`, integer step/window).
tests/queries/0_stateless/03254_timeseries_to_grid_aggregate_function_sparse.sql	Disable randomized parallel replicas to keep type comparisons stable.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

alexey-milovidov · 2026-05-17T18:42:09Z

This was fixed by #105146. Let's update the branch.

PedroTadim · 2026-05-18T12:41:11Z

Hi! Please mention it closes #104459

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.

clickhouse-gh · 2026-05-18T22:00:36Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	84.10%	84.10%	+0.00%
Functions	91.40%	91.40%	+0.00%
Branches	76.60%	76.60%	+0.00%

Changed lines: 52.50% (105/200) | lost baseline coverage: 9 line(s) · Uncovered code

Full report · Diff report

vitlibar · 2026-05-20T12:54:45Z

@davenger Can you please take a look?

davenger · 2026-05-20T13:11:41Z

@@ -51,6 +51,10 @@ class AggregateFunctionTimeseriesBase :
        return Traits::getName();
    }

+    /// Timeseries parameters may carry DecimalField (from toDateTime64(...) casts), whose
+    /// default printed form collides with String literals — so we print parameters with ::Type.
+    bool shouldPrintParametersWithTypes() const override { return true; }


Won't this break compatibility with the tables created by older version that have AggregateFunctionState columns?

Old types are still completely parseable. There can be a problem if a new version writes a type like AggregateFunction(timeSeriesInstantRateToGrid('1734004810'::Decimal64(3), '1734004860'::Decimal64(3), 10, 60), Array(DateTime64(3, 'UTC')), Array(Float64)), and then an older version will try to read it (because older versions don't know how to parse parameters like '1734004810'::Decimal64(3). However timeSeries* aggregate functions are experimental, so I suppose that's OK

…vert-103428-resubmit-preserve-parameter-types-in-timeseries-agg-functions Preserve parameter types in timeseries aggregate functions, add type annotations

Revert "Revert "Resubmit: Preserve parameter types in timeseries aggr…

f6373ac

…egate functions""

clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label May 13, 2026

clickhouse-gh Bot reviewed May 13, 2026

View reviewed changes

Comment thread tests/queries/0_stateless/03254_timeseries_instant_value_aggregate_functions.sql Outdated

vitlibar mentioned this pull request May 14, 2026

Fix LOGICAL_ERROR in AggregateFunctionArray for timeSeries*ToGrid family #104769

Closed

1 task

This was referenced May 14, 2026

Push join key filters into MergeTree index during recursive CTE evaluation #97254

Draft

Resubmit: Preserve parameter types in timeseries aggregate functions #103428

Merged

vitlibar requested a review from Copilot May 15, 2026 20:25

Copilot started reviewing on behalf of vitlibar May 15, 2026 20:25 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

Comment thread src/AggregateFunctions/AggregateFunctionFactory.cpp Outdated

Comment thread src/AggregateFunctions/TimeSeries/AggregateFunctionTimeseriesLinearRegression.h Outdated

clickhouse-gh Bot reviewed May 15, 2026

View reviewed changes

Comment thread src/AggregateFunctions/AggregateFunctionFactory.cpp Outdated

vitlibar requested a review from Copilot May 15, 2026 20:42

Copilot started reviewing on behalf of vitlibar May 15, 2026 20:42 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

vitlibar force-pushed the revert-104672-revert-103428-resubmit-preserve-parameter-types-in-timeseries-agg-functions branch from 8bc3475 to f1f9bc6 Compare May 15, 2026 22:56

vitlibar requested a review from Copilot May 15, 2026 22:57

Copilot started reviewing on behalf of vitlibar May 15, 2026 22:57 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

clickhouse-gh Bot reviewed May 15, 2026

View reviewed changes

Comment thread src/AggregateFunctions/AggregateFunctionFactory.cpp Outdated

vitlibar requested a review from Copilot May 15, 2026 23:10

Copilot started reviewing on behalf of vitlibar May 15, 2026 23:10 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

vitlibar added 3 commits May 16, 2026 18:30

Revert excessive settings

b47068d

Add comments

904e0e7

Align chasserts for checking invariant

d6bf250

vitlibar force-pushed the revert-104672-revert-103428-resubmit-preserve-parameter-types-in-timeseries-agg-functions branch from 9ff74b5 to d6bf250 Compare May 16, 2026 16:35

vitlibar requested a review from Copilot May 16, 2026 16:35

Copilot started reviewing on behalf of vitlibar May 16, 2026 16:35 View session

Copilot AI reviewed May 16, 2026

View reviewed changes

vitlibar force-pushed the revert-104672-revert-103428-resubmit-preserve-parameter-types-in-timeseries-agg-functions branch from 6ad6eea to 44466ed Compare May 17, 2026 21:38

groeneai mentioned this pull request May 18, 2026

TimeSeries: enable dynamic remote-write routing via URL table prefix #93833

Open

1 task

vitlibar mentioned this pull request May 18, 2026

Logical error: AggregateFunctionArray: parameters mismatch between Array wrapper 'A' and nested function 'B'. Wrapper has C parameter(s): [D], nested function has E parameter(s): [F] (STID: 4870-4f07) #104459

Closed

vitlibar force-pushed the revert-104672-revert-103428-resubmit-preserve-parameter-types-in-timeseries-agg-functions branch 2 times, most recently from 1967d6c to b963749 Compare May 18, 2026 18:25

timeSeries aggregate functions now print parameters with their types

6e4db42

vitlibar force-pushed the revert-104672-revert-103428-resubmit-preserve-parameter-types-in-timeseries-agg-functions branch from b963749 to 6e4db42 Compare May 18, 2026 19:04

vitlibar changed the title ~~Resubmit #2: Preserve parameter types in timeseries aggregate functions~~ Preserve parameter types in timeseries aggregate functions, add type annotations May 18, 2026

vitlibar requested a review from Copilot May 18, 2026 20:10

Copilot started reviewing on behalf of vitlibar May 18, 2026 20:10 View session

Copilot AI reviewed May 18, 2026

View reviewed changes

vitlibar marked this pull request as ready for review May 18, 2026 21:45

vitlibar added the 🍃 green ci 🌿 Fixing flaky tests in CI label May 18, 2026

vitlibar requested a review from davenger May 20, 2026 12:55

davenger reviewed May 20, 2026

View reviewed changes

vdimir mentioned this pull request May 21, 2026

Populate ProfileEvents for executable_pool UDFs #105010

Merged

1 task

groeneai mentioned this pull request May 22, 2026

Logical error: 'parameters == nested_func->getParameters()' (STID: 2508-495a) #102993

Closed

nikitamikhaylov approved these changes May 22, 2026

View reviewed changes

nikitamikhaylov self-assigned this May 22, 2026

nikitamikhaylov added this pull request to the merge queue May 22, 2026

Merged via the queue into master with commit 4a0750d May 22, 2026
170 checks passed

nikitamikhaylov deleted the revert-104672-revert-103428-resubmit-preserve-parameter-types-in-timeseries-agg-functions branch May 22, 2026 16:10

robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label May 22, 2026

vitlibar mentioned this pull request Jun 1, 2026

Make the list of aggregation functions dropping their parameters explicit #106245

Draft

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

vitlibar commented May 13, 2026 • edited by robot-clickhouse Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Version info

Uh oh!

clickhouse-gh Bot commented May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Findings

Tests

Final Verdict

Uh oh!

clickhouse-gh Bot May 13, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot May 15, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot May 16, 2026

Choose a reason for hiding this comment

Uh oh!

clickhouse-gh Bot May 17, 2026

Choose a reason for hiding this comment

Uh oh!

vitlibar Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

alexey-milovidov commented May 17, 2026

Uh oh!

PedroTadim commented May 18, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clickhouse-gh Bot commented May 18, 2026

LLVM Coverage Report

Uh oh!

vitlibar commented May 20, 2026

Uh oh!

davenger May 20, 2026

Choose a reason for hiding this comment

Uh oh!

vitlibar commented May 13, 2026 •

edited by robot-clickhouse

Loading

clickhouse-gh Bot commented May 13, 2026 •

edited

Loading

vitlibar May 20, 2026 •

edited

Loading