From 58e47d9cc897a170041b5e48642b076e1e476bdb Mon Sep 17 00:00:00 2001 From: happenlee Date: Thu, 10 Sep 2026 19:01:10 +0800 Subject: [PATCH 01/16] [fix](be) Reject incompatible aggregate state parameters ### What problem does this PR solve? Issue Number: N/A Related PR: N/A Problem Summary: AggState types describe argument types but do not include constant parameter values. For example, `topn_merge` over a `UNION ALL` of `topn_state('a', 1)` and `topn_state('a', 3)` can combine incompatible states. TopN previously overwrote the destination N/capacity, making results depend on merge order. Other configurable aggregates silently adopted incompatible settings; legacy percentile arrays could index beyond the destination state when quantile counts differed. Reject incompatible populated states with `INVALID_ARGUMENT`, using `UNLIKELY` for the mismatch paths. Apply the same invariant to TopN variants, histograms, percentile variants, limited collect, group_concat, intersect_count, exponential moving average, sequence functions and both window_funnel implementations. Preserve empty-state identity and reset behavior, initialize approximate percentile digests with the source compression, and keep serialization formats unchanged. ### Release note Merging aggregate states with incompatible parameters now returns an error instead of producing incorrect results or risking an out-of-bounds access. ### Check List (For Author) - Test: - [x] Regression test: `test_agg_state_parameters` covers both `_merge` and `_union`, both input orders, and 39 incompatible parameter pairs. - [x] Unit Test: 56 tests passed, including parameter compatibility, serialized merge, empty states, reset and compatible-state results, plus existing related aggregate tests. - Local validation: BE ASAN and FE builds, the regression suite (156 expected errors), clang-format 16, and BE header hygiene passed. Local BE startup required OpenBLAS `USE_OPENMP=FALSE` to avoid a toolchain initialization crash; this build-cache setting is not part of the patch. Full clang-tidy is blocked by the existing unmatched `NOLINTEND` in `be/src/core/types.h`; diagnostics on the changed code have been addressed. - Behavior changed: - [x] Yes. Incompatible aggregate state parameters fail with `INVALID_ARGUMENT`; matching states retain their semantics. - Does this need documentation? - [x] No. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label --- .../aggregate/aggregate_function_collect.h | 13 + .../exprs/aggregate/aggregate_function_ema.h | 14 +- .../aggregate_function_group_concat.h | 5 + .../aggregate/aggregate_function_histogram.h | 13 +- .../aggregate_function_linear_histogram.h | 11 +- .../aggregate_function_orthogonal_bitmap.h | 6 + .../aggregate/aggregate_function_percentile.h | 34 ++- .../aggregate_function_percentile_reservoir.h | 11 +- .../aggregate_function_sequence_match.h | 9 +- .../exprs/aggregate/aggregate_function_topn.h | 18 +- .../aggregate_function_window_funnel.h | 9 + .../aggregate_function_window_funnel_v2.h | 11 +- be/src/util/bitmap_intersect.h | 24 ++ be/src/util/reservoir_sampler.h | 2 + .../aggregate/agg_state_parameters_test.cpp | 280 ++++++++++++++++++ .../test_agg_state_parameters.groovy | 85 ++++++ 16 files changed, 512 insertions(+), 33 deletions(-) create mode 100644 be/test/exprs/aggregate/agg_state_parameters_test.cpp create mode 100644 regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy diff --git a/be/src/exprs/aggregate/aggregate_function_collect.h b/be/src/exprs/aggregate/aggregate_function_collect.h index 52894f5e05c2f9..25c04d30167bd6 100644 --- a/be/src/exprs/aggregate/aggregate_function_collect.h +++ b/be/src/exprs/aggregate/aggregate_function_collect.h @@ -28,6 +28,7 @@ #include #include +#include "common/exception.h" #include "core/assert_cast.h" #include "core/column/column.h" #include "core/column/column_array.h" @@ -444,6 +445,18 @@ class AggregateFunctionCollect final Arena& arena) const override { auto& data = this->data(place); const auto& rhs_data = this->data(rhs); + if constexpr (HasLimit) { + if (rhs_data.max_size == -1) { + return; + } + if (data.max_size != -1) { + if (UNLIKELY(data.max_size != rhs_data.max_size)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "{} aggregate states have incompatible limits: {} vs {}", + get_name(), data.max_size, rhs_data.max_size); + } + } + } if constexpr (ENABLE_ARENA) { data.merge(rhs_data, arena); } else { diff --git a/be/src/exprs/aggregate/aggregate_function_ema.h b/be/src/exprs/aggregate/aggregate_function_ema.h index 30ab8f0745cdcd..f83640d8b78d28 100644 --- a/be/src/exprs/aggregate/aggregate_function_ema.h +++ b/be/src/exprs/aggregate/aggregate_function_ema.h @@ -23,6 +23,7 @@ #include #include +#include "common/exception.h" #include "core/assert_cast.h" #include "core/column/column_vector.h" #include "core/data_type/data_type_number.h" @@ -86,12 +87,17 @@ struct ExponentialMovingAverageData { } void merge(const ExponentialMovingAverageData& rhs) { - double hd = half_decay != 0.0 ? half_decay : rhs.half_decay; - if (hd == 0.0) { + if (rhs.half_decay == 0.0) { return; } - half_decay = hd; - merge_point(rhs, hd); + if (half_decay == 0.0) { + half_decay = rhs.half_decay; + } else if (UNLIKELY(half_decay != rhs.half_decay)) { + throw Exception( + ErrorCode::INVALID_ARGUMENT, + "exponential_moving_average aggregate states have incompatible half decay"); + } + merge_point(rhs, half_decay); } double get() const { diff --git a/be/src/exprs/aggregate/aggregate_function_group_concat.h b/be/src/exprs/aggregate/aggregate_function_group_concat.h index 0fbd654b74cbbd..52d662d46e603c 100644 --- a/be/src/exprs/aggregate/aggregate_function_group_concat.h +++ b/be/src/exprs/aggregate/aggregate_function_group_concat.h @@ -22,6 +22,7 @@ #include #include +#include "common/exception.h" #include "core/assert_cast.h" #include "core/column/column_string.h" #include "core/data_type/data_type_string.h" @@ -73,6 +74,10 @@ struct AggregateFunctionGroupConcatData { separator = rhs.separator; data.assign(rhs.data); } else { + if (UNLIKELY(separator != rhs.separator)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "group_concat aggregate states have incompatible separators"); + } auto offset = data.size(); auto delta_size = separator.size() + rhs.data.size(); diff --git a/be/src/exprs/aggregate/aggregate_function_histogram.h b/be/src/exprs/aggregate/aggregate_function_histogram.h index 671acb8646debf..2025b5898bc3e5 100644 --- a/be/src/exprs/aggregate/aggregate_function_histogram.h +++ b/be/src/exprs/aggregate/aggregate_function_histogram.h @@ -50,7 +50,10 @@ struct AggregateFunctionHistogramData { void set_parameters(size_t input_max_num_buckets) { max_num_buckets = input_max_num_buckets; } - void reset() { ordered_map.clear(); } + void reset() { + ordered_map.clear(); + max_num_buckets = BUCKET_NUM_INIT_VALUE; + } void add(const StringRef& value, const UInt64& number = 1) { std::string data = value.to_string(); @@ -78,7 +81,13 @@ struct AggregateFunctionHistogramData { return; } - max_num_buckets = rhs.max_num_buckets; + if (!max_num_buckets) { + max_num_buckets = rhs.max_num_buckets; + } else if (UNLIKELY(max_num_buckets != rhs.max_num_buckets)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "histogram aggregate states have incompatible bucket counts: {} vs {}", + max_num_buckets, rhs.max_num_buckets); + } for (auto rhs_it : rhs.ordered_map) { auto lhs_it = ordered_map.find(rhs_it.first); diff --git a/be/src/exprs/aggregate/aggregate_function_linear_histogram.h b/be/src/exprs/aggregate/aggregate_function_linear_histogram.h index 22f91e0f4d6c1f..6bc2393d3a1b5b 100644 --- a/be/src/exprs/aggregate/aggregate_function_linear_histogram.h +++ b/be/src/exprs/aggregate/aggregate_function_linear_histogram.h @@ -24,6 +24,7 @@ #include #include +#include "common/exception.h" #include "core/column/column_string.h" #include "core/column/column_vector.h" #include "core/data_type/data_type_decimal.h" @@ -90,8 +91,14 @@ struct AggregateFunctionLinearHistogramData { return; } - interval = rhs.interval; - offset = rhs.offset; + if (interval == 0) { + interval = rhs.interval; + offset = rhs.offset; + } else if (UNLIKELY(interval != rhs.interval || offset != rhs.offset)) { + throw Exception( + ErrorCode::INVALID_ARGUMENT, + "linear_histogram aggregate states have incompatible interval or offset"); + } for (const auto& [key, count] : rhs.buckets) { buckets[key] += count; diff --git a/be/src/exprs/aggregate/aggregate_function_orthogonal_bitmap.h b/be/src/exprs/aggregate/aggregate_function_orthogonal_bitmap.h index c2b7f6a92f3523..c89397df5bed35 100644 --- a/be/src/exprs/aggregate/aggregate_function_orthogonal_bitmap.h +++ b/be/src/exprs/aggregate/aggregate_function_orthogonal_bitmap.h @@ -150,6 +150,12 @@ struct AggIntersectCount : public AggOrthBitmapBaseData { if (rhs.first_init) { return; } + if (!AggOrthBitmapBaseData::first_init) { + if (UNLIKELY(!AggOrthBitmapBaseData::bitmap.has_same_keys(rhs.bitmap))) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "intersect_count aggregate states have incompatible filter values"); + } + } AggOrthBitmapBaseData::bitmap.merge(rhs.bitmap); AggOrthBitmapBaseData::first_init = false; } diff --git a/be/src/exprs/aggregate/aggregate_function_percentile.h b/be/src/exprs/aggregate/aggregate_function_percentile.h index cfd8e239538b21..be2ca3f726741f 100644 --- a/be/src/exprs/aggregate/aggregate_function_percentile.h +++ b/be/src/exprs/aggregate/aggregate_function_percentile.h @@ -28,6 +28,7 @@ #include #include +#include "common/exception.h" #include "core/assert_cast.h" #include "core/column/column.h" #include "core/column/column_array.h" @@ -139,17 +140,18 @@ struct PercentileApproxState { if (!rhs.init_flag) { return; } - if (init_flag) { - DCHECK(digest.get() != nullptr); - digest->merge(rhs.digest.get()); - } else { + if (!init_flag) { + target_quantile = rhs.target_quantile; + compressions = rhs.compressions; digest = TDigest::create_unique(compressions); - digest->merge(rhs.digest.get()); init_flag = true; + } else if (UNLIKELY(target_quantile != rhs.target_quantile || + compressions != rhs.compressions)) { + throw Exception( + ErrorCode::INVALID_ARGUMENT, + "percentile_approx aggregate states have incompatible quantile or compression"); } - if (target_quantile == PercentileApproxState::INIT_QUANTILE) { - target_quantile = rhs.target_quantile; - } + digest->merge(rhs.digest.get()); } void add(double source) { digest->add(static_cast(source)); } @@ -414,7 +416,8 @@ struct PercentileApproxArrayState { } init_flag = true; } else { - if (compressions != rhs.compressions || levels.quantiles != rhs.levels.quantiles) { + if (UNLIKELY(compressions != rhs.compressions || + levels.quantiles != rhs.levels.quantiles)) { throw Exception( ErrorCode::INVALID_ARGUMENT, "percentile_approx_array aggregate states have incompatible quantiles " @@ -642,14 +645,14 @@ struct PercentileState { int size_num = cast_set(rhs.vec_quantile.size()); if (!inited_flag) { vec_counts.resize(size_num); - vec_quantile.resize(size_num, -1); + vec_quantile = rhs.vec_quantile; inited_flag = true; + } else if (UNLIKELY(vec_quantile != rhs.vec_quantile)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "percentile aggregate states have incompatible quantiles"); } for (int i = 0; i < size_num; ++i) { - if (vec_quantile[i] == -1.0) { - vec_quantile[i] = rhs.vec_quantile[i]; - } vec_counts[i].merge(&(rhs.vec_counts[i])); } } @@ -736,8 +739,9 @@ struct PercentileExactState { if (!inited_flag) { levels = rhs.levels; inited_flag = true; - } else { - levels.merge(rhs.levels); + } else if (UNLIKELY(levels.quantiles != rhs.levels.quantiles)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "percentile aggregate states have incompatible quantiles"); } _append(rhs.values.data(), rhs.values.size()); } diff --git a/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h b/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h index e7ac0160749d5b..2c2bd0d3e8fc98 100644 --- a/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h +++ b/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h @@ -21,6 +21,7 @@ #include +#include "common/exception.h" #include "core/column/column_vector.h" #include "core/data_type/data_type_number.h" #include "core/data_type/primitive_type.h" @@ -44,7 +45,15 @@ struct QuantileReservoirSampler { } void merge(const QuantileReservoirSampler& rhs) { - level = rhs.level; + if (rhs.data.empty()) { + return; + } + if (data.empty()) { + level = rhs.level; + } else if (UNLIKELY(level != rhs.level)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "percentile_reservoir aggregate states have incompatible quantiles"); + } data.merge(rhs.data); } diff --git a/be/src/exprs/aggregate/aggregate_function_sequence_match.h b/be/src/exprs/aggregate/aggregate_function_sequence_match.h index 85b17358bf60d9..72e90a4f4fcff8 100644 --- a/be/src/exprs/aggregate/aggregate_function_sequence_match.h +++ b/be/src/exprs/aggregate/aggregate_function_sequence_match.h @@ -37,6 +37,7 @@ #include #include +#include "common/exception.h" #include "common/logging.h" #include "core/assert_cast.h" #include "core/column/column_string.h" @@ -119,6 +120,12 @@ struct AggregateFunctionSequenceMatchData final { void merge(const AggregateFunctionSequenceMatchData& other) { if (other.events_list.empty()) return; + if (!init_flag) { + init(other.pattern, other.arg_count); + } else if (UNLIKELY(pattern != other.pattern || arg_count != other.arg_count)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "sequence aggregate states have incompatible patterns or event counts"); + } events_list.insert(std::end(events_list), std::begin(other.events_list), std::end(other.events_list)); sorted = false; @@ -648,8 +655,6 @@ class AggregateFunctionSequenceBase void merge(AggregateDataPtr __restrict place, ConstAggregateDataPtr rhs, Arena&) const override { - const std::string pattern = this->data(rhs).get_pattern(); - this->data(place).init(pattern, this->data(rhs).get_arg_count()); this->data(place).merge(this->data(rhs)); } diff --git a/be/src/exprs/aggregate/aggregate_function_topn.h b/be/src/exprs/aggregate/aggregate_function_topn.h index 1c2edb7e9dc07a..8e35276186d233 100644 --- a/be/src/exprs/aggregate/aggregate_function_topn.h +++ b/be/src/exprs/aggregate/aggregate_function_topn.h @@ -30,6 +30,7 @@ #include #include +#include "common/exception.h" #include "core/assert_cast.h" #include "core/column/column.h" #include "core/column/column_array.h" @@ -85,8 +86,15 @@ struct AggregateFunctionTopNData { return; } - top_num = rhs.top_num; - capacity = rhs.capacity; + if (!top_num) { + top_num = rhs.top_num; + capacity = rhs.capacity; + } else if (UNLIKELY(top_num != rhs.top_num || capacity != rhs.capacity)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "topn aggregate states have incompatible parameters: " + "({}, {}) vs ({}, {}) (N, capacity)", + top_num, capacity, rhs.top_num, rhs.capacity); + } bool lhs_full = (counter_map.size() >= capacity); bool rhs_full = (rhs.counter_map.size() >= capacity); @@ -194,7 +202,11 @@ struct AggregateFunctionTopNData { } } - void reset() { counter_map.clear(); } + void reset() { + counter_map.clear(); + top_num = 0; + capacity = 0; + } int top_num = 0; uint64_t capacity = 0; diff --git a/be/src/exprs/aggregate/aggregate_function_window_funnel.h b/be/src/exprs/aggregate/aggregate_function_window_funnel.h index 2ecacd5b083715..28ffa997d5200f 100644 --- a/be/src/exprs/aggregate/aggregate_function_window_funnel.h +++ b/be/src/exprs/aggregate/aggregate_function_window_funnel.h @@ -295,6 +295,15 @@ struct WindowFunnelState { if (other.events_list.empty()) { return; } + + if (events_list.empty()) { + window = other.window; + window_funnel_mode = other.window_funnel_mode; + } else if (UNLIKELY(window != other.window || + window_funnel_mode != other.window_funnel_mode)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "window_funnel aggregate states have incompatible window or mode"); + } events_list.dt.insert(std::end(events_list.dt), std::begin(other.events_list.dt), std::end(other.events_list.dt)); for (size_t i = 0; i < event_count; i++) { diff --git a/be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h b/be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h index af2fac10e73009..f958a8d08a88ea 100644 --- a/be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h +++ b/be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h @@ -184,9 +184,16 @@ struct WindowFunnelStateV2 { } if (events_list.empty()) { + window = other.window; + window_funnel_mode = other.window_funnel_mode; events_list = other.events_list; sorted = other.sorted; } else { + if (UNLIKELY(window != other.window || + window_funnel_mode != other.window_funnel_mode)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "window_funnel aggregate states have incompatible window or mode"); + } const auto prefix_size = events_list.size(); events_list.insert(std::end(events_list), std::begin(other.events_list), std::end(other.events_list)); @@ -199,10 +206,6 @@ struct WindowFunnelStateV2 { } event_count = event_count > 0 ? event_count : other.event_count; - window = window != WINDOW_UNSET ? window : other.window; - window_funnel_mode = window_funnel_mode == WindowFunnelMode::INVALID - ? other.window_funnel_mode - : window_funnel_mode; } void write(BufferWritable& out) const { diff --git a/be/src/util/bitmap_intersect.h b/be/src/util/bitmap_intersect.h index 7e9d0308843338..65d4a80b45d6e8 100644 --- a/be/src/util/bitmap_intersect.h +++ b/be/src/util/bitmap_intersect.h @@ -169,6 +169,18 @@ struct BitmapIntersect { } } + bool has_same_keys(const BitmapIntersect& other) const { + if (_bitmaps.size() != other._bitmaps.size()) { + return false; + } + for (const auto& [key, bitmap] : _bitmaps) { + if (!other._bitmaps.contains(key)) { + return false; + } + } + return true; + } + void merge(const BitmapIntersect& other) { for (auto& kv : other._bitmaps) { if (_bitmaps.find(kv.first) != _bitmaps.end()) { @@ -259,6 +271,18 @@ struct BitmapIntersect { } } + bool has_same_keys(const BitmapIntersect& other) const { + if (_bitmaps.size() != other._bitmaps.size()) { + return false; + } + for (const auto& [key, bitmap] : _bitmaps) { + if (!other._bitmaps.contains(key)) { + return false; + } + } + return true; + } + void merge(const BitmapIntersect& other) { for (auto& kv : other._bitmaps) { if (_bitmaps.find(kv.first) != _bitmaps.end()) { diff --git a/be/src/util/reservoir_sampler.h b/be/src/util/reservoir_sampler.h index a54a2cf89b0fea..79b456c694b1a8 100644 --- a/be/src/util/reservoir_sampler.h +++ b/be/src/util/reservoir_sampler.h @@ -349,6 +349,8 @@ class ReservoirSampler { return samples[left_index] * left_coef + samples[right_index] * right_coef; } + bool empty() const { return total_values == 0; } + void merge(const ReservoirSampler& b) { if (sample_count != b.sample_count) { throw doris::Exception(ErrorCode::INTERNAL_ERROR, diff --git a/be/test/exprs/aggregate/agg_state_parameters_test.cpp b/be/test/exprs/aggregate/agg_state_parameters_test.cpp new file mode 100644 index 00000000000000..82266b44eaa341 --- /dev/null +++ b/be/test/exprs/aggregate/agg_state_parameters_test.cpp @@ -0,0 +1,280 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include + +#include "common/exception.h" +#include "core/column/column_array.h" +#include "core/data_type/data_type_array.h" +#include "core/data_type/data_type_bitmap.h" +#include "core/data_type/data_type_date_or_datetime_v2.h" +#include "core/data_type/data_type_nullable.h" +#include "core/data_type/data_type_number.h" +#include "core/data_type/data_type_string.h" +#include "exprs/aggregate/aggregate_function_simple_factory.h" +#include "testutil/column_helper.h" + +namespace doris { +namespace { +using Arguments = std::vector; + +template +ColumnWithTypeAndName argument(typename T::FieldType value) { + return {ColumnHelper::create_column({value}), std::make_shared(), ""}; +} + +ColumnWithTypeAndName quantiles(const std::vector& values) { + auto nested = ColumnHelper::create_nullable_column( + values, std::vector(values.size(), 0)); + auto offsets = ColumnArray::ColumnOffsets::create(); + offsets->insert_value(values.size()); + auto type = std::make_shared(make_nullable(std::make_shared())); + return {ColumnArray::create(nested, std::move(offsets)), type, ""}; +} + +template +void expect_incompatible(Operation&& operation) { + try { + operation(); + FAIL() << "Expected incompatible state parameters to be rejected"; + } catch (const Exception& e) { + EXPECT_EQ(e.code(), ErrorCode::INVALID_ARGUMENT); + EXPECT_NE(e.to_string().find("incompatible"), std::string::npos); + } +} + +class StateParameterChecks { +public: + explicit StateParameterChecks(AggregateFunctionPtr function) : _function(std::move(function)) {} + + ~StateParameterChecks() { + for (auto* place : _places) { + _function->destroy(place); + } + } + + void check_mismatch(const Arguments& first, const Arguments& second) { + auto* destination = create(first); + auto* source = create(second); + expect_incompatible([&] { _function->merge(destination, source, _arena); }); + auto serialized = serialize(source); + expect_incompatible([&] { + _function->deserialize_and_merge_from_column(destination, *serialized, _arena); + }); + } + + void check_empty_and_reset(const Arguments& first, const Arguments& second) { + auto* destination = create(first); + auto* source = create(); + EXPECT_NO_THROW(_function->merge(destination, source, _arena)); + _function->reset(destination); + auto serialized = serialize(create(second)); + EXPECT_NO_THROW( + _function->deserialize_and_merge_from_column(destination, *serialized, _arena)); + EXPECT_TRUE(ColumnHelper::column_equal(result(destination), result(create(second)))); + } + + void check_compatible(const Arguments& arguments) { + auto* destination = create(); + auto serialized = serialize(create(arguments)); + EXPECT_NO_THROW( + _function->deserialize_and_merge_from_column(destination, *serialized, _arena)); + EXPECT_NO_THROW( + _function->deserialize_and_merge_from_column(destination, *serialized, _arena)); + auto* expected = create(arguments); + add(expected, arguments); + EXPECT_TRUE(ColumnHelper::column_equal(result(destination), result(expected))); + } + +private: + AggregateDataPtr create() { + auto* place = reinterpret_cast(_arena.alloc(_function->size_of_data())); + _function->create(place); + _places.push_back(place); + return place; + } + + AggregateDataPtr create(const Arguments& arguments) { + auto* place = create(); + add(place, arguments); + return place; + } + + void add(AggregateDataPtr place, const Arguments& arguments) { + std::vector columns; + for (const auto& arg : arguments) { + columns.push_back(arg.column.get()); + } + _function->add(place, columns.data(), 0, _arena); + } + + MutableColumnPtr serialize(AggregateDataPtr place) { + auto column = _function->create_serialize_column(); + _function->serialize_without_key_to_column(place, *column); + return column; + } + + ColumnPtr result(AggregateDataPtr place) { + auto column = _function->get_return_type()->create_column(); + _function->insert_result_into(place, *column); + return column; + } + + AggregateFunctionPtr _function; + Arena _arena; + std::vector _places; +}; + +void check_parameters(const std::string& name, const Arguments& first, const Arguments& second) { + SCOPED_TRACE(name); + DataTypes types; + for (const auto& arg : first) { + types.push_back(arg.type); + } + auto function = AggregateFunctionSimpleFactory::instance().get( + name, types, nullptr, false, BeExecVersionManager::get_newest_version()); + ASSERT_NE(function, nullptr); + function->set_version(BeExecVersionManager::get_newest_version()); + StateParameterChecks checks(function); + for (bool reverse : {false, true}) { + SCOPED_TRACE(reverse); + const auto& lhs_args = reverse ? second : first; + const auto& rhs_args = reverse ? first : second; + checks.check_mismatch(lhs_args, rhs_args); + checks.check_empty_and_reset(lhs_args, rhs_args); + checks.check_compatible(lhs_args); + } +} +} // namespace + +TEST(AggregateStateParametersTest, TopN) { + const auto value = argument("a"); + check_parameters("topn", {value, argument(1)}, + {value, argument(3)}); + for (const auto& name : {"topn", "topn_array", "topn_weighted"}) { + Arguments args {value}; + if (std::string(name) == "topn_weighted") { + args.push_back(argument(1)); + } + auto first = args; + auto second = args; + first.insert(first.end(), {argument(3), argument(2)}); + second.insert(second.end(), {argument(3), argument(5)}); + check_parameters(name, first, second); + // Equal capacities do not make different N values compatible. + second = args; + second.insert(second.end(), {argument(1), argument(6)}); + check_parameters(name, first, second); + } + check_parameters("topn_array", {argument(1), argument(1)}, + {argument(1), argument(3)}); +} + +TEST(AggregateStateParametersTest, Histograms) { + auto value = argument(7); + check_parameters("histogram", {value, argument(1)}, + {value, argument(3)}); + check_parameters("linear_histogram", {value, argument(2)}, + {value, argument(3)}); + check_parameters("linear_histogram", + {value, argument(2), argument(0)}, + {value, argument(2), argument(1)}); +} + +TEST(AggregateStateParametersTest, Percentiles) { + auto value = argument(7); + for (const auto& name : + {"percentile", "percentile_v2", "percentile_approx", "percentile_reservoir"}) { + check_parameters(name, {value, argument(0)}, + {value, argument(1)}); + } + for (const auto& name : + {"percentile_array", "percentile_array_v2", "percentile_approx_array"}) { + check_parameters(name, {value, quantiles({0.25})}, {value, quantiles({0.75})}); + check_parameters(name, {value, quantiles({0.25})}, {value, quantiles({0.25, 0.75})}); + check_parameters(name, {value, quantiles({})}, {value, quantiles({0.25})}); + } + check_parameters("percentile_approx", + {value, argument(0.5), argument(2048)}, + {value, argument(0.5), argument(4096)}); + check_parameters("percentile_approx_weighted", + {value, argument(1), argument(0.25)}, + {value, argument(1), argument(0.75)}); + check_parameters("percentile_approx_weighted", + {value, argument(1), argument(0.5), + argument(2048)}, + {value, argument(1), argument(0.5), + argument(4096)}); +} + +TEST(AggregateStateParametersTest, CollectAndConcat) { + for (const auto& name : {"collect_list", "collect_set"}) { + for (const auto& value : {argument(7), argument("a")}) { + check_parameters(name, {value, argument(1)}, + {value, argument(3)}); + } + } + check_parameters("collect_list", {quantiles({0.5}), argument(1)}, + {quantiles({0.5}), argument(3)}); + auto value = argument("a"); + check_parameters("group_concat", {value, argument(",")}, + {value, argument(";")}); +} + +TEST(AggregateStateParametersTest, IntersectCount) { + auto bitmap_column = ColumnBitmap::create(); + bitmap_column->insert_value(BitmapValue {uint64_t(1)}); + ColumnWithTypeAndName bitmap {std::move(bitmap_column), std::make_shared(), ""}; + auto value = argument(1); + check_parameters("intersect_count", {bitmap, value, argument(1)}, + {bitmap, value, argument(2)}); + auto text = argument("a"); + check_parameters("intersect_count", {bitmap, text, argument("a")}, + {bitmap, text, argument("b")}); +} + +TEST(AggregateStateParametersTest, ExponentialMovingAverage) { + auto value = argument(7); + auto time = argument(1); + check_parameters("exponential_moving_average", {argument(1), value, time}, + {argument(2), value, time}); +} + +TEST(AggregateStateParametersTest, SequenceAndWindowFunnel) { + DateV2Value time; + time.unchecked_set_time(2024, 1, 1, 0, 0, 0, 0); + auto timestamp = argument(time); + auto yes = argument(1); + auto no = argument(0); + for (const auto& name : {"sequence_match", "sequence_count"}) { + check_parameters(name, {argument("(?1)"), timestamp, yes, no}, + {argument("(?2)"), timestamp, yes, no}); + } + for (const auto& name : {"window_funnel_v1", "window_funnel_v2"}) { + check_parameters(name, + {argument(1), argument("default"), + timestamp, yes, no}, + {argument(3), argument("default"), + timestamp, yes, no}); + check_parameters(name, + {argument(1), argument("default"), + timestamp, yes, no}, + {argument(1), argument("fixed"), timestamp, + yes, no}); + } +} +} // namespace doris diff --git a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy new file mode 100644 index 00000000000000..cbcbca2b88f8ca --- /dev/null +++ b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy @@ -0,0 +1,85 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("test_agg_state_parameters") { + sql "set enable_agg_state=true" + + // Each pair has the same AggState type but incompatible configuration values. + def cases = [ + ["topn", "'a', 1", "'a', 3"], + ["topn", "'a', 3, 2", "'a', 3, 5"], + ["topn", "'a', 1, 6", "'a', 3, 2"], + ["topn_array", "1, 1", "1, 3"], + ["topn_array", "1, 3, 2", "1, 3, 5"], + ["topn_array", "'a', 1, 6", "'a', 3, 2"], + ["topn_weighted", "1, 1, 1", "1, 1, 3"], + ["topn_weighted", "1, 1, 3, 2", "1, 1, 3, 5"], + ["histogram", "7, 1", "7, 3"], + ["linear_histogram", "7, 2", "7, 3"], + ["linear_histogram", "7, 2, 0", "7, 2, 1"], + ["percentile", "7, 0.25", "7, 0.75"], + ["percentile_array", "7, [0.25]", "7, [0.75]"], + ["percentile_array", "7, [0.25]", "7, [0.25, 0.75]"], + ["percentile_array", "7, cast([] as array)", "7, [0.25]"], + ["percentile_approx", "7, 0.25", "7, 0.75"], + ["percentile_approx", "7, 0.5, 2048", "7, 0.5, 4096"], + ["percentile_approx_array", "7, [0.25]", "7, [0.75]"], + ["percentile_approx_array", "7, [0.25], 2048", "7, [0.25], 4096"], + ["percentile_approx_weighted", "7, 1, 0.25", "7, 1, 0.75"], + ["percentile_approx_weighted", "7, 1, 0.5, 2048", "7, 1, 0.5, 4096"], + ["percentile_reservoir", "7, 0.25", "7, 0.75"], + ["collect_list", "7, 1", "7, 3"], + ["collect_list", "'a', 1", "'a', 3"], + ["collect_list", "[7], 1", "[7], 3"], + ["collect_set", "7, 1", "7, 3"], + ["collect_set", "'a', 1", "'a', 3"], + ["intersect_count", "to_bitmap(1), 1, 1", "to_bitmap(1), 1, 2"], + ["intersect_count", "to_bitmap(1), 'a', 'a'", "to_bitmap(1), 'a', 'b'"], + ["group_concat", "'a', ','", "'a', ';'"], + ["exponential_moving_average", "1, 7, 1", "2, 7, 1"], + ["sequence_match", "'(?1)', non_nullable(cast('2024-01-01' as datetime)), true, false", + "'(?2)', non_nullable(cast('2024-01-01' as datetime)), true, false"], + ["sequence_count", "'(?1)', non_nullable(cast('2024-01-01' as datetime)), true, false", + "'(?2)', non_nullable(cast('2024-01-01' as datetime)), true, false"] + ] + // Use STRING for both modes to keep the AggState argument types identical. + for (def function : ["window_funnel", "window_funnel_v1", "window_funnel_v2"]) { + cases.add([function, "1, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), true, false", + "3, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), true, false"]) + cases.add([function, "1, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), true, false", + "1, cast('fixed' as string), non_nullable(cast('2024-01-01' as datetime)), true, false"]) + } + + for (def entry : cases) { + def function = entry[0] + for (def args : [[entry[1], entry[2]], [entry[2], entry[1]]]) { + for (def suffix : ["merge", "union"]) { + test { + sql """ + SELECT ${function}_${suffix}(s) + FROM ( + SELECT ${function}_state(${args[0]}) AS s + UNION ALL + SELECT ${function}_state(${args[1]}) AS s + ) states + """ + exception "aggregate states have incompatible" + } + } + } + } +} From f33cb430bcd9a5f8e1ae8fd6545243581a3054c3 Mon Sep 17 00:00:00 2001 From: happenlee Date: Thu, 10 Sep 2026 21:34:05 +0800 Subject: [PATCH 02/16] [fix](be) Ignore collect states with negative limits during merge ### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: Collect state merging only skipped the -1 initialization marker, while other negative limits could be adopted or compared against nonnegative limits. Treat every negative-limit source state as non-contributing during merge. In the serialized merge path, negative sources are skipped before they can initialize the fresh destination. ### Release note Collect state merging now ignores source states with any negative limit. ### Check List (For Author) - Test: Manual review of the one-line condition change; clang-format 16, repository format check and BE build hygiene passed. No build or runtime tests run for this incremental review fix. - Behavior changed: Yes. Source states with limits below -1 are skipped during merge. - Does this need documentation: No --- be/src/exprs/aggregate/aggregate_function_collect.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/be/src/exprs/aggregate/aggregate_function_collect.h b/be/src/exprs/aggregate/aggregate_function_collect.h index 25c04d30167bd6..7c25b75c04fa5b 100644 --- a/be/src/exprs/aggregate/aggregate_function_collect.h +++ b/be/src/exprs/aggregate/aggregate_function_collect.h @@ -446,7 +446,7 @@ class AggregateFunctionCollect final auto& data = this->data(place); const auto& rhs_data = this->data(rhs); if constexpr (HasLimit) { - if (rhs_data.max_size == -1) { + if (rhs_data.max_size < 0) { return; } if (data.max_size != -1) { From d7a9d5ec9f15e75cbae8bc2c5420b7617c03f183 Mon Sep 17 00:00:00 2001 From: happenlee Date: Thu, 10 Sep 2026 21:38:42 +0800 Subject: [PATCH 03/16] [fix](fe) Require a constant collect_set limit ### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: CollectSet accepted a varying limit expression even though the backend state retains the first observed limit. Add the same FE constant-argument check as CollectList so ordinary collect_set and its state/combine combinators reject a nonconstant limit before execution. Add expected-error regression cases for all three entry points. ### Release note collect_set now requires its optional limit argument to be constant, matching collect_list. ### Check List (For Author) - Test: FE Checkstyle passed with zero violations; git diff --check passed. Regression cases added but not executed in this incremental update; no FE build run. - Behavior changed: Yes. Nonconstant collect_set limits are rejected during analysis. - Does this need documentation: No --- .../trees/expressions/functions/agg/CollectSet.java | 10 ++++++++++ .../agg_state/test_agg_state_parameters.groovy | 10 ++++++++++ 2 files changed, 20 insertions(+) diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/CollectSet.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/CollectSet.java index 66f17dd5c16fa3..fc434f41718002 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/CollectSet.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/CollectSet.java @@ -18,6 +18,7 @@ package org.apache.doris.nereids.trees.expressions.functions.agg; import org.apache.doris.catalog.FunctionSignature; +import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.functions.ExplicitlyCastableSignature; import org.apache.doris.nereids.trees.expressions.literal.ArrayLiteral; @@ -108,4 +109,13 @@ public List getSignatures() { public Expression resultForEmptyInput() { return new ArrayLiteral(new ArrayList<>(), this.getDataType()); } + + @Override + public void checkLegalityBeforeTypeCoercion() { + if (arity() == 2 && !getArgument(1).isConstant()) { + throw new AnalysisException( + "collect_set requires second parameter must be a constant: " + + this.toSql()); + } + } } diff --git a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy index cbcbca2b88f8ca..4416bc2b083a07 100644 --- a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy +++ b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy @@ -18,6 +18,16 @@ suite("test_agg_state_parameters") { sql "set enable_agg_state=true" + for (def function : ["collect_set", "collect_set_state", "collect_set_combine"]) { + test { + sql """ + SELECT ${function}(number, cast(number AS int)) + FROM numbers("number" = "3") + """ + exception "collect_set requires second parameter must be a constant" + } + } + // Each pair has the same AggState type but incompatible configuration values. def cases = [ ["topn", "'a', 1", "'a', 3"], From afd1b807d3079ce1a82b90e67fce5c227c747941 Mon Sep 17 00:00:00 2001 From: happenlee Date: Thu, 10 Sep 2026 22:40:01 +0800 Subject: [PATCH 04/16] [doc](be) Clarify EMA zero half-decay state semantics ### What problem does this PR solve? Related PR: #67805 Problem Summary: Clarify that a zero EMA half-decay returns zero and marks a non-contributing serialized aggregate state. Such states are ignored by merge and union; incompatible nonzero half-decays still raise an error. Preserve the existing implementation and document this exception to parameter validation. ### Release note None ### Check List (For Author) - Test: No need to test (documentation and source comments only); C++ formatting, header hygiene and git diff --check passed. - Behavior changed: No - Does this need documentation: Yes (included in docs/exponential-moving-average.md) --- .../exprs/aggregate/aggregate_function_ema.h | 4 +++ docs/exponential-moving-average.md | 31 +++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 docs/exponential-moving-average.md diff --git a/be/src/exprs/aggregate/aggregate_function_ema.h b/be/src/exprs/aggregate/aggregate_function_ema.h index f83640d8b78d28..523d1896970d85 100644 --- a/be/src/exprs/aggregate/aggregate_function_ema.h +++ b/be/src/exprs/aggregate/aggregate_function_ema.h @@ -57,6 +57,10 @@ class IColumn; * - value: numeric column to average * - timeunit: numeric time index (not raw timestamp; use intDiv if needed) * Returns DOUBLE. + * + * A zero half_decay returns 0 and is also the empty-state marker. Serialized + * zero-half-decay states do not contribute to _merge/_union and do not trigger + * a half-decay mismatch. Compatibility checks apply to nonzero half decays. */ struct ExponentialMovingAverageData { double value = 0.0; diff --git a/docs/exponential-moving-average.md b/docs/exponential-moving-average.md new file mode 100644 index 00000000000000..8b9b3f8651d892 --- /dev/null +++ b/docs/exponential-moving-average.md @@ -0,0 +1,31 @@ +# Exponential moving average: zero half-decay semantics + +`exponential_moving_average(half_decay, value, timeunit)` accepts a constant numeric +half-decay. A half-decay of `0` is a special case: the aggregate returns `0` for +non-null input rows, rather than evaluating the exponential-decay formula. + +For example: + +```sql +SELECT exponential_moving_average(0, 7, 1); +-- 0 +``` + +A zero half-decay also marks an empty aggregate state. When combining serialized +states with `exponential_moving_average_merge` or +`exponential_moving_average_union`, states whose half-decay is `0` are ignored. +This applies even when the state was created from non-null input rows. + +Consequently: + +- Combining states with half-decays `0` and `1` uses the contributing state with + half-decay `1`, without a parameter-mismatch error, in either input order. +- Combining contributing states with different nonzero half-decays, such as `1` + and `2`, raises an incompatible-half-decay error. +- Combining non-null states whose half-decays are all `0` yields `0` when the + aggregate is finalized. + +This documents the existing zero-half-decay behavior. Zero is not a distinct +contributing configuration for aggregate-state compatibility checks. These rules +concern zero half-decay; they do not define NaN as an empty state or change SQL +NULL handling. From 9fb011ef1860597a646edb9faa135d6cbd1546c6 Mon Sep 17 00:00:00 2001 From: happenlee Date: Thu, 10 Sep 2026 23:09:20 +0800 Subject: [PATCH 05/16] [fix](be) Reject NaN EMA half-decay on state and result output ### What problem does this PR solve? Related PR: #67805 Problem Summary: Two EMA states configured with NaN can fail the half-decay compatibility check because NaN is unequal to itself. Treat NaN half-decay as unsupported and reject it when serializing a state or finalizing a result. Use one shared check at the output boundaries, preserving add/merge behavior and the existing zero-half-decay semantics. ### Release note exponential_moving_average rejects NaN half-decay when serializing an aggregate state or producing a final result. ### Check List (For Author) - Test: Unit Test: all 8 AggregateStateParametersTest tests passed with ASAN. Formatting, header hygiene and git diff --check passed. Added SQL regression cases were not run. Clang-tidy encountered pre-existing diagnostics in core/types.h and unchanged aggregate code. - Behavior changed: Yes (NaN half-decay now errors at state/result output) - Does this need documentation: Yes (updated docs/exponential-moving-average.md) --- .../exprs/aggregate/aggregate_function_ema.h | 9 +++++ .../aggregate/agg_state_parameters_test.cpp | 37 +++++++++++++++++++ docs/exponential-moving-average.md | 9 +++-- .../test_agg_state_parameters.groovy | 8 ++++ 4 files changed, 60 insertions(+), 3 deletions(-) diff --git a/be/src/exprs/aggregate/aggregate_function_ema.h b/be/src/exprs/aggregate/aggregate_function_ema.h index 523d1896970d85..972390c8ee3314 100644 --- a/be/src/exprs/aggregate/aggregate_function_ema.h +++ b/be/src/exprs/aggregate/aggregate_function_ema.h @@ -105,6 +105,7 @@ struct ExponentialMovingAverageData { } double get() const { + check_half_decay(); if (half_decay == 0.0) { return 0.0; } @@ -112,6 +113,7 @@ struct ExponentialMovingAverageData { } void write(BufferWritable& buf) const { + check_half_decay(); buf.write_binary(value); buf.write_binary(time); buf.write_binary(half_decay); @@ -123,6 +125,13 @@ struct ExponentialMovingAverageData { buf.read_binary(half_decay); } + void check_half_decay() const { + if (UNLIKELY(std::isnan(half_decay))) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "exponential_moving_average half decay must not be NaN"); + } + } + void reset() { value = 0.0; time = 0.0; diff --git a/be/test/exprs/aggregate/agg_state_parameters_test.cpp b/be/test/exprs/aggregate/agg_state_parameters_test.cpp index 82266b44eaa341..13d2a811d4fb23 100644 --- a/be/test/exprs/aggregate/agg_state_parameters_test.cpp +++ b/be/test/exprs/aggregate/agg_state_parameters_test.cpp @@ -17,6 +17,8 @@ #include +#include + #include "common/exception.h" #include "core/column/column_array.h" #include "core/data_type/data_type_array.h" @@ -100,6 +102,24 @@ class StateParameterChecks { EXPECT_TRUE(ColumnHelper::column_equal(result(destination), result(expected))); } + void check_invalid_outputs(const Arguments& arguments, const std::string& message) { + auto* state = create(arguments); + for (bool serialize_state : {false, true}) { + SCOPED_TRACE(serialize_state); + try { + if (serialize_state) { + serialize(state); + } else { + result(state); + } + FAIL() << "Expected invalid state parameters to be rejected"; + } catch (const Exception& e) { + EXPECT_EQ(e.code(), ErrorCode::INVALID_ARGUMENT); + EXPECT_NE(e.to_string().find(message), std::string::npos); + } + } + } + private: AggregateDataPtr create() { auto* place = reinterpret_cast(_arena.alloc(_function->size_of_data())); @@ -254,6 +274,23 @@ TEST(AggregateStateParametersTest, ExponentialMovingAverage) { {argument(2), value, time}); } +TEST(AggregateStateParametersTest, ExponentialMovingAverageNaNOutputs) { + auto type = std::make_shared(); + auto function = AggregateFunctionSimpleFactory::instance().get( + "exponential_moving_average", {type, type, type}, nullptr, false, + BeExecVersionManager::get_newest_version()); + ASSERT_NE(function, nullptr); + StateParameterChecks checks(function); + auto value = argument(7); + auto time = argument(1); + checks.check_invalid_outputs( + {argument(std::numeric_limits::quiet_NaN()), value, time}, + "half decay must not be NaN"); + for (double half_decay : {0.0, 1.0}) { + checks.check_compatible({argument(half_decay), value, time}); + } +} + TEST(AggregateStateParametersTest, SequenceAndWindowFunnel) { DateV2Value time; time.unchecked_set_time(2024, 1, 1, 0, 0, 0, 0); diff --git a/docs/exponential-moving-average.md b/docs/exponential-moving-average.md index 8b9b3f8651d892..335eb2e626c2cb 100644 --- a/docs/exponential-moving-average.md +++ b/docs/exponential-moving-average.md @@ -26,6 +26,9 @@ Consequently: aggregate is finalized. This documents the existing zero-half-decay behavior. Zero is not a distinct -contributing configuration for aggregate-state compatibility checks. These rules -concern zero half-decay; they do not define NaN as an empty state or change SQL -NULL handling. +contributing configuration for aggregate-state compatibility checks. SQL NULL +handling is unchanged. + +A NaN half-decay is unsupported. Serializing an aggregate state or finalizing its +result raises `exponential_moving_average half decay must not be NaN`. This check +applies to the half-decay parameter, not to the input value or the computed result. diff --git a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy index 4416bc2b083a07..5d3e232c696b75 100644 --- a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy +++ b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy @@ -28,6 +28,14 @@ suite("test_agg_state_parameters") { } } + for (def function : ["exponential_moving_average", "exponential_moving_average_state", + "exponential_moving_average_combine"]) { + test { + sql "SELECT ${function}(cast('NaN' AS double), cast(7 AS double), cast(1 AS double))" + exception "half decay must not be NaN" + } + } + // Each pair has the same AggState type but incompatible configuration values. def cases = [ ["topn", "'a', 1", "'a', 3"], From 349ed2a83e6251cd369a9c63407ffcca69926ab8 Mon Sep 17 00:00:00 2001 From: happenlee Date: Fri, 11 Sep 2026 00:34:05 +0800 Subject: [PATCH 06/16] [refactor](be) Exclude linear_histogram from aggregate state validation ### What problem does this PR solve? Related PR: #67805 Problem Summary: Remove linear_histogram from this change's scope. Restore its header exactly to the PR base and remove the corresponding parameter mismatch checks from the BE unit test and SQL regression suite. The function's parameter contract requires separate work; the other aggregate state validations remain. ### Release note None ### Check List (For Author) - Test: Manual test: verified the header matches the PR base byte for byte and no linear_histogram references remain in the PR diff. Formatting, header hygiene and git diff --check passed. Runtime tests were not rerun because this restores the base implementation and removes its newly added tests. - Behavior changed: No (relative to the PR base) - Does this need documentation: No --- .../aggregate/aggregate_function_linear_histogram.h | 11 ++--------- be/test/exprs/aggregate/agg_state_parameters_test.cpp | 7 +------ .../agg_state/test_agg_state_parameters.groovy | 2 -- 3 files changed, 3 insertions(+), 17 deletions(-) diff --git a/be/src/exprs/aggregate/aggregate_function_linear_histogram.h b/be/src/exprs/aggregate/aggregate_function_linear_histogram.h index 6bc2393d3a1b5b..22f91e0f4d6c1f 100644 --- a/be/src/exprs/aggregate/aggregate_function_linear_histogram.h +++ b/be/src/exprs/aggregate/aggregate_function_linear_histogram.h @@ -24,7 +24,6 @@ #include #include -#include "common/exception.h" #include "core/column/column_string.h" #include "core/column/column_vector.h" #include "core/data_type/data_type_decimal.h" @@ -91,14 +90,8 @@ struct AggregateFunctionLinearHistogramData { return; } - if (interval == 0) { - interval = rhs.interval; - offset = rhs.offset; - } else if (UNLIKELY(interval != rhs.interval || offset != rhs.offset)) { - throw Exception( - ErrorCode::INVALID_ARGUMENT, - "linear_histogram aggregate states have incompatible interval or offset"); - } + interval = rhs.interval; + offset = rhs.offset; for (const auto& [key, count] : rhs.buckets) { buckets[key] += count; diff --git a/be/test/exprs/aggregate/agg_state_parameters_test.cpp b/be/test/exprs/aggregate/agg_state_parameters_test.cpp index 13d2a811d4fb23..843805d2bf859d 100644 --- a/be/test/exprs/aggregate/agg_state_parameters_test.cpp +++ b/be/test/exprs/aggregate/agg_state_parameters_test.cpp @@ -204,15 +204,10 @@ TEST(AggregateStateParametersTest, TopN) { {argument(1), argument(3)}); } -TEST(AggregateStateParametersTest, Histograms) { +TEST(AggregateStateParametersTest, Histogram) { auto value = argument(7); check_parameters("histogram", {value, argument(1)}, {value, argument(3)}); - check_parameters("linear_histogram", {value, argument(2)}, - {value, argument(3)}); - check_parameters("linear_histogram", - {value, argument(2), argument(0)}, - {value, argument(2), argument(1)}); } TEST(AggregateStateParametersTest, Percentiles) { diff --git a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy index 5d3e232c696b75..d4d260927ce928 100644 --- a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy +++ b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy @@ -47,8 +47,6 @@ suite("test_agg_state_parameters") { ["topn_weighted", "1, 1, 1", "1, 1, 3"], ["topn_weighted", "1, 1, 3, 2", "1, 1, 3, 5"], ["histogram", "7, 1", "7, 3"], - ["linear_histogram", "7, 2", "7, 3"], - ["linear_histogram", "7, 2, 0", "7, 2, 1"], ["percentile", "7, 0.25", "7, 0.75"], ["percentile_array", "7, [0.25]", "7, [0.75]"], ["percentile_array", "7, [0.25]", "7, [0.25, 0.75]"], From 243c526d7cb70b5815cd6fb2e3b605cbb34eabd6 Mon Sep 17 00:00:00 2001 From: happenlee Date: Fri, 11 Sep 2026 01:21:43 +0800 Subject: [PATCH 07/16] [fix](fe) Reject NaN percentile reservoir levels ### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: The percentile_reservoir FE legality check accepts NaN literals because both out-of-range comparisons are false. Negate the inclusive valid range instead so NaN is rejected with the existing AnalysisException, and document the floating-point comparison behavior. Valid levels, including zero and one, remain accepted. ### Release note Reject NaN percentile_reservoir quantile literals during query analysis. ### Check List (For Author) - Test: Manual Java predicate checks covering NaN, infinities, out-of-range values, negative zero, endpoints and an interior value; FE Checkstyle and git diff --check. FE unit tests and SQL regression tests were not run. - Behavior changed: Yes. FE rejects NaN quantile literals. - Does this need documentation: No. Enforces the existing [0, 1] parameter range. --- .../trees/expressions/functions/agg/PercentileReservoir.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoir.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoir.java index 5e91e794064220..6d6903345e9229 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoir.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoir.java @@ -74,7 +74,8 @@ public void checkLegalityBeforeTypeCoercion() { } if (levelArgument instanceof Literal) { double value = ((Literal) levelArgument).getDouble(); - if (value < 0 || value > 1) { + // Negate the valid range to reject NaN, which makes both < 0 and > 1 false. + if (!(value >= 0 && value <= 1)) { throw new AnalysisException( "percentile_reservoir level must be in [0, 1], but got " + value + ": " + this.toSql()); } From fdf2a529775044ccc05b894648efe9586e78f18c Mon Sep 17 00:00:00 2001 From: happenlee Date: Fri, 11 Sep 2026 01:34:37 +0800 Subject: [PATCH 08/16] [fix](be) Validate configured eventless sequence states ### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: Sequence states initialized by all-false event rows retain their pattern but contain no events. The merge path skipped these states as sources while checking their configuration as destinations, making direct merge validation asymmetric and allowing serialized eventless inputs to bypass compatibility checks. Ignore only uninitialized sources, adopt or validate established configuration before the no-event fast return, and keep serialized states with zero arguments uninitialized. Incompatible patterns now fail regardless of which configured state contains events or which input is merged first. The serialized layout is unchanged. ### Release note Reject incompatible sequence_match and sequence_count aggregate-state patterns even when one or both configured states contain no events. ### Check List (For Author) - Test: Unit Test - 20 ASAN tests passed via run-be-ut.sh, including aggregate parameter checks and existing sequence tests. Added expected-error SQL regression cases for both merge/union input orders; these SQL cases were not executed. clang-format 16, BE header hygiene and git diff --check passed. Full clang-tidy is blocked by existing core/types.h and unchanged sequence-header diagnostics. - Behavior changed: Yes. Configured eventless states retain parameter constraints during merge; truly uninitialized states remain identity elements. - Does this need documentation: No. --- .../aggregate_function_sequence_match.h | 19 ++++-- .../aggregate/agg_state_parameters_test.cpp | 60 +++++++++++++++++++ .../test_agg_state_parameters.groovy | 8 +++ 3 files changed, 83 insertions(+), 4 deletions(-) diff --git a/be/src/exprs/aggregate/aggregate_function_sequence_match.h b/be/src/exprs/aggregate/aggregate_function_sequence_match.h index 72e90a4f4fcff8..c9838999722427 100644 --- a/be/src/exprs/aggregate/aggregate_function_sequence_match.h +++ b/be/src/exprs/aggregate/aggregate_function_sequence_match.h @@ -118,7 +118,10 @@ struct AggregateFunctionSequenceMatchData final { } void merge(const AggregateFunctionSequenceMatchData& other) { - if (other.events_list.empty()) return; + // All-false event rows still establish a pattern that must match during merge. + if (!other.init_flag) { + return; + } if (!init_flag) { init(other.pattern, other.arg_count); @@ -126,6 +129,10 @@ struct AggregateFunctionSequenceMatchData final { throw Exception(ErrorCode::INVALID_ARGUMENT, "sequence aggregate states have incompatible patterns or event counts"); } + if (other.events_list.empty()) { + return; + } + events_list.insert(std::end(events_list), std::begin(other.events_list), std::end(other.events_list)); sorted = false; @@ -664,9 +671,13 @@ class AggregateFunctionSequenceBase void deserialize(AggregateDataPtr __restrict place, BufferReadable& buf, Arena&) const override { - this->data(place).read(buf); - const std::string pattern = this->data(place).get_pattern(); - this->data(place).init(pattern, this->data(place).get_arg_count()); + auto& state = AggregateFunctionSequenceBase::data(place); + state.read(buf); + // A serialized uninitialized state has no arguments and must stay uninitialized. + if (state.get_arg_count() == 0) { + return; + } + state.init(state.get_pattern(), state.get_arg_count()); } void check_input_columns_type(const IColumn** columns) const override { diff --git a/be/test/exprs/aggregate/agg_state_parameters_test.cpp b/be/test/exprs/aggregate/agg_state_parameters_test.cpp index 843805d2bf859d..3308e57c041fea 100644 --- a/be/test/exprs/aggregate/agg_state_parameters_test.cpp +++ b/be/test/exprs/aggregate/agg_state_parameters_test.cpp @@ -77,6 +77,35 @@ class StateParameterChecks { expect_incompatible([&] { _function->deserialize_and_merge_from_column(destination, *serialized, _arena); }); + auto* fresh_destination = create(); + auto serialized_first = serialize(create(first)); + _function->deserialize_and_merge_from_column(fresh_destination, *serialized_first, _arena); + expect_incompatible([&] { + _function->deserialize_and_merge_from_column(fresh_destination, *serialized, _arena); + }); + } + + void check_sequence_merge(const Arguments& initial, const Arguments& incoming, + bool serialized) { + auto* destination = create(); + auto* source = create(); + auto* expected = create(); + if (!initial.empty()) { + add(destination, initial); + add(expected, initial); + } + if (!incoming.empty()) { + add(source, incoming); + add(expected, incoming); + } + if (serialized) { + auto column = serialize(source); + EXPECT_NO_THROW( + _function->deserialize_and_merge_from_column(destination, *column, _arena)); + } else { + EXPECT_NO_THROW(_function->merge(destination, source, _arena)); + } + EXPECT_TRUE(ColumnHelper::column_equal(result(destination), result(expected))); } void check_empty_and_reset(const Arguments& first, const Arguments& second) { @@ -309,4 +338,35 @@ TEST(AggregateStateParametersTest, SequenceAndWindowFunnel) { yes, no}); } } + +TEST(AggregateStateParametersTest, SequenceEventlessParameters) { + DateV2Value time; + time.unchecked_set_time(2024, 1, 1, 0, 0, 0, 0); + auto timestamp = argument(time); + auto yes = argument(1); + auto no = argument(0); + for (const auto& name : {"sequence_match", "sequence_count"}) { + SCOPED_TRACE(name); + Arguments eventless {argument("(?1)"), timestamp, no, no}; + check_parameters(name, eventless, {argument("(?2)"), timestamp, yes, no}); + check_parameters(name, eventless, {argument("(?2)"), timestamp, no, no}); + DataTypes types; + for (const auto& arg : eventless) { + types.push_back(arg.type); + } + auto function = AggregateFunctionSimpleFactory::instance().get( + name, types, nullptr, false, BeExecVersionManager::get_newest_version()); + ASSERT_NE(function, nullptr); + function->set_version(BeExecVersionManager::get_newest_version()); + StateParameterChecks checks(function); + Arguments contributing {argument("(?1)"), timestamp, yes, no}; + for (const auto& initial : {Arguments {}, eventless, contributing}) { + for (const auto& incoming : {Arguments {}, eventless, contributing}) { + for (bool serialized : {false, true}) { + checks.check_sequence_merge(initial, incoming, serialized); + } + } + } + } +} } // namespace doris diff --git a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy index d4d260927ce928..35c3e6e0e993dc 100644 --- a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy +++ b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy @@ -72,6 +72,14 @@ suite("test_agg_state_parameters") { ["sequence_count", "'(?1)', non_nullable(cast('2024-01-01' as datetime)), true, false", "'(?2)', non_nullable(cast('2024-01-01' as datetime)), true, false"] ] + // All-false event rows retain their pattern, including when both states have no events. + for (def function : ["sequence_match", "sequence_count"]) { + for (def event : ["true", "false"]) { + cases.add([function, + "'(?1)', non_nullable(cast('2024-01-01' as datetime)), false, false", + "'(?2)', non_nullable(cast('2024-01-01' as datetime)), ${event}, false"]) + } + } // Use STRING for both modes to keep the AggState argument types identical. for (def function : ["window_funnel", "window_funnel_v1", "window_funnel_v2"]) { cases.add([function, "1, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), true, false", From a8b018b9ef62be2b8f32becc097acf037ba08691 Mon Sep 17 00:00:00 2001 From: happenlee Date: Fri, 11 Sep 2026 02:07:03 +0800 Subject: [PATCH 09/16] [fix](fe) Require constant window funnel parameters ### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: Window funnel states overwrite window and mode on each raw add, so column-valued configurations can mix events under different settings before merge-time validation. Require constant window and mode in FE for both implementations, including ordinary, state and combine calls. Add FE and regression coverage, and FE unit coverage for the earlier reservoir NaN validation fix. ### Release note Window funnel calls with nonconstant window or mode parameters now fail during analysis. ### Check List (For Author) - Test: FE unit tests (5 passed), regression tests (test_agg_state_parameters, window_funnel, window_funnel_v2 passed), BE ASAN and FE build including Checkstyle passed; latest related BE ASAN run passed 20 tests. - Behavior changed: Yes, window and mode must be constant. - Does this need documentation: No --- .../functions/agg/WindowFunnel.java | 6 ++ .../functions/agg/WindowFunnelV2.java | 6 ++ .../agg/PercentileReservoirParameterTest.java | 62 ++++++++++++++ .../agg/WindowFunnelParameterTest.java | 84 +++++++++++++++++++ .../test_agg_state_parameters.groovy | 22 +++++ 5 files changed, 180 insertions(+) create mode 100644 fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoirParameterTest.java create mode 100644 fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/agg/WindowFunnelParameterTest.java diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/WindowFunnel.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/WindowFunnel.java index 9f152821c2fef0..a70004b320356b 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/WindowFunnel.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/WindowFunnel.java @@ -93,6 +93,12 @@ public void checkLegalityBeforeTypeCoercion() { if (!getArgumentType(1).isStringLikeType()) { throw new AnalysisException("The mode params of " + functionName + " function must be string"); } + if (!getArgument(0).isConstant()) { + throw new AnalysisException("The window parameter of " + functionName + " must be a constant"); + } + if (!getArgument(1).isConstant()) { + throw new AnalysisException("The mode parameter of " + functionName + " must be a constant"); + } if (!getArgumentType(2).isDateLikeType()) { throw new AnalysisException("The 3rd param of " + functionName + " function must be DATE, DATETIME, TIMESTAMP_NS or TIMESTAMPTZ"); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/WindowFunnelV2.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/WindowFunnelV2.java index fabca97f7448ed..e23e4354c7f71d 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/WindowFunnelV2.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/WindowFunnelV2.java @@ -102,6 +102,12 @@ public void checkLegalityBeforeTypeCoercion() { if (!getArgumentType(1).isStringLikeType()) { throw new AnalysisException("The mode params of " + functionName + " function must be string"); } + if (!getArgument(0).isConstant()) { + throw new AnalysisException("The window parameter of " + functionName + " must be a constant"); + } + if (!getArgument(1).isConstant()) { + throw new AnalysisException("The mode parameter of " + functionName + " must be a constant"); + } if (!getArgumentType(2).isDateLikeType()) { throw new AnalysisException("The 3rd param of " + functionName + " function must be DATE, DATETIME, TIMESTAMP_NS or TIMESTAMPTZ"); diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoirParameterTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoirParameterTest.java new file mode 100644 index 00000000000000..4aa87268171927 --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/agg/PercentileReservoirParameterTest.java @@ -0,0 +1,62 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.nereids.trees.expressions.functions.agg; + +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.SlotReference; +import org.apache.doris.nereids.trees.expressions.functions.combinator.CombineCombinator; +import org.apache.doris.nereids.trees.expressions.functions.combinator.StateCombinator; +import org.apache.doris.nereids.trees.expressions.literal.DoubleLiteral; +import org.apache.doris.nereids.types.DoubleType; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.Arrays; +import java.util.List; + +public class PercentileReservoirParameterTest { + @Test + void testRejectNaNAndOutOfRangeLevels() { + for (double level : new double[] {Double.NaN, Double.NEGATIVE_INFINITY, + Double.POSITIVE_INFINITY, -0.1, 1.1}) { + for (Expression expression : variants(level)) { + AnalysisException exception = Assertions.assertThrows(AnalysisException.class, + expression::checkLegalityBeforeTypeCoercion); + Assertions.assertTrue(exception.getMessage().contains("level must be in [0, 1]")); + } + } + } + + @Test + void testAcceptEndpointsAndInteriorLevels() { + for (double level : new double[] {-0.0, 0.0, 0.5, 1.0}) { + for (Expression expression : variants(level)) { + Assertions.assertDoesNotThrow(expression::checkLegalityBeforeTypeCoercion); + } + } + } + + private List variants(double level) { + PercentileReservoir function = new PercentileReservoir( + SlotReference.of("value", DoubleType.INSTANCE), new DoubleLiteral(level)); + return Arrays.asList(function, StateCombinator.create(function), + new CombineCombinator(function.getArguments(), function)); + } +} diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/agg/WindowFunnelParameterTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/agg/WindowFunnelParameterTest.java new file mode 100644 index 00000000000000..1524ad48640bed --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/agg/WindowFunnelParameterTest.java @@ -0,0 +1,84 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.nereids.trees.expressions.functions.agg; + +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.SlotReference; +import org.apache.doris.nereids.trees.expressions.functions.combinator.CombineCombinator; +import org.apache.doris.nereids.trees.expressions.functions.combinator.StateCombinator; +import org.apache.doris.nereids.trees.expressions.literal.BigIntLiteral; +import org.apache.doris.nereids.trees.expressions.literal.VarcharLiteral; +import org.apache.doris.nereids.types.BigIntType; +import org.apache.doris.nereids.types.BooleanType; +import org.apache.doris.nereids.types.DateTimeV2Type; +import org.apache.doris.nereids.types.StringType; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.Arrays; +import java.util.List; + +public class WindowFunnelParameterTest { + @Test + void testRejectWindowColumn() { + for (AggregateFunction function : functions(SlotReference.of("window", BigIntType.INSTANCE), + new VarcharLiteral("default"))) { + assertRejected(function, "window"); + } + } + + @Test + void testRejectModeColumn() { + for (AggregateFunction function : functions(new BigIntLiteral(10), + SlotReference.of("mode", StringType.INSTANCE))) { + assertRejected(function, "mode"); + } + } + + @Test + void testAcceptConstantsWithEventColumns() { + for (AggregateFunction function : functions(new BigIntLiteral(10), new VarcharLiteral("default"))) { + for (Expression expression : variants(function)) { + Assertions.assertDoesNotThrow(expression::checkLegalityBeforeTypeCoercion); + } + } + } + + private void assertRejected(AggregateFunction function, String parameter) { + for (Expression expression : variants(function)) { + AnalysisException exception = Assertions.assertThrows(AnalysisException.class, + expression::checkLegalityBeforeTypeCoercion); + Assertions.assertEquals("The " + parameter + " parameter of " + function.getName() + + " must be a constant", exception.getMessage()); + } + } + + private List variants(AggregateFunction function) { + return Arrays.asList(function, StateCombinator.create(function), + new CombineCombinator(function.getArguments(), function)); + } + + private List functions(Expression window, Expression mode) { + Expression timestamp = SlotReference.of("ts", DateTimeV2Type.SYSTEM_DEFAULT); + Expression event = SlotReference.of("event", BooleanType.INSTANCE); + return Arrays.asList(new WindowFunnel(window, mode, timestamp, event), + new WindowFunnelV2(window, mode, timestamp, event)); + } +} diff --git a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy index 35c3e6e0e993dc..e5182620eee5f6 100644 --- a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy +++ b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy @@ -18,6 +18,28 @@ suite("test_agg_state_parameters") { sql "set enable_agg_state=true" + for (def function : ["window_funnel", "window_funnel_v1", "window_funnel_v2"]) { + def implementation = function == "window_funnel" ? "window_funnel_v2" : function + for (def suffix : ["", "_state", "_combine"]) { + test { + sql """ + SELECT ${function}${suffix}(number, 'default', + cast('2024-01-01' AS datetime), true, false) + FROM numbers("number" = "3") + """ + exception "The window parameter of ${implementation} must be a constant" + } + test { + sql """ + SELECT ${function}${suffix}(10, if(number = 0, 'default', 'fixed'), + cast('2024-01-01' AS datetime), true, false) + FROM numbers("number" = "3") + """ + exception "The mode parameter of ${implementation} must be a constant" + } + } + } + for (def function : ["collect_set", "collect_set_state", "collect_set_combine"]) { test { sql """ From 21892a505bf8789c96f101531c7cfb6c9d55364c Mon Sep 17 00:00:00 2001 From: happenlee Date: Fri, 11 Sep 2026 17:57:24 +0800 Subject: [PATCH 10/16] [fix](be) Preserve configuration in eventless funnel states ### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: WindowFunnelStateV2 records window and mode for all-false input but stores no events. Its merge identity branches previously ignored or overwrote this configuration, accepting incompatible aggregate states in either operand order. Track initialization separately, compare configured parameters before empty-payload handling, and clear configuration on reset. Preserve the serialized field layout using a configured-empty tag in the existing sorted field, which legacy readers already interpret as sorted. Recover initialized legacy states from their parameters or event payload. Extend existing unit and regression coverage for both operand orders, serialized/direct merges, fresh/reset states, aliases and legacy framing. ### Release note All-false window_funnel and window_funnel_v2 states retain their configured window and mode and reject merges with incompatible parameters. ### Check List (For Author) - Test: Unit Test / Regression test / Manual test - 58 related BE ASAN unit tests passed. - test_agg_state_parameters, window_funnel and window_funnel_v2 regression suites passed. - The original binary failed the new all-false mismatch regression as expected. - BE ASAN build, clang-format 16, header hygiene and whitespace checks passed. - clang-tidy reports existing diagnostics; none remain on changed lines. - Behavior changed: Yes, configured eventless funnel states reject incompatible merges. - Does this need documentation: No, the parameter contract is described in the PR release note. --- .../aggregate_function_window_funnel_v2.h | 39 +++++--- .../aggregate/agg_state_parameters_test.cpp | 56 +++++++++++- .../aggregate/vec_window_funnel_v2_test.cpp | 88 +++++++++++++++++++ .../test_agg_state_parameters.groovy | 7 ++ 4 files changed, 176 insertions(+), 14 deletions(-) diff --git a/be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h b/be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h index f958a8d08a88ea..a7c23e262411fb 100644 --- a/be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h +++ b/be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h @@ -124,6 +124,7 @@ struct WindowFunnelStateV2 { int event_count = 0; int64_t window = WINDOW_UNSET; WindowFunnelMode window_funnel_mode = WindowFunnelMode::INVALID; + bool initialized = false; bool sorted = true; std::vector events_list; @@ -131,6 +132,9 @@ struct WindowFunnelStateV2 { WindowFunnelStateV2(int arg_event_count) : event_count(arg_event_count) {} void reset() { + window = WINDOW_UNSET; + window_funnel_mode = WindowFunnelMode::INVALID; + initialized = false; events_list.clear(); sorted = true; } @@ -138,6 +142,7 @@ struct WindowFunnelStateV2 { void add(const IColumn** arg_columns, ssize_t row_num, int64_t win, WindowFunnelMode mode) { window = win; window_funnel_mode = mode; + initialized = true; auto timestamp = assert_cast::ColumnType&, TypeCheckOnRelease::DISABLE>(*arg_columns[2]) @@ -179,21 +184,28 @@ struct WindowFunnelStateV2 { } void merge(const WindowFunnelStateV2& other) { - if (other.events_list.empty()) { + if (!other.initialized) { return; } - - if (events_list.empty()) { + // All-false input establishes configuration without retaining any events. + if (!initialized) { window = other.window; window_funnel_mode = other.window_funnel_mode; + event_count = other.event_count; + initialized = true; + } else if (UNLIKELY(window != other.window || + window_funnel_mode != other.window_funnel_mode)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "window_funnel aggregate states have incompatible window or mode"); + } + + if (other.events_list.empty()) { + return; + } + if (events_list.empty()) { events_list = other.events_list; sorted = other.sorted; } else { - if (UNLIKELY(window != other.window || - window_funnel_mode != other.window_funnel_mode)) { - throw Exception(ErrorCode::INVALID_ARGUMENT, - "window_funnel aggregate states have incompatible window or mode"); - } const auto prefix_size = events_list.size(); events_list.insert(std::end(events_list), std::begin(other.events_list), std::end(other.events_list)); @@ -204,8 +216,6 @@ struct WindowFunnelStateV2 { merge_events_list(events_list, prefix_size, sorted, other.sorted); sorted = true; } - - event_count = event_count > 0 ? event_count : other.event_count; } void write(BufferWritable& out) const { @@ -213,7 +223,10 @@ struct WindowFunnelStateV2 { write_var_int(window, out); write_var_int(static_cast>(window_funnel_mode), out); - write_var_int(sorted ? 1 : 0, out); + // Tag configured eventless states in the existing sorted field. Legacy readers + // interpret every nonzero value as sorted, so the layout remains compatible. + const auto sorted_flag = static_cast(sorted); + write_var_int(initialized && events_list.empty() ? 2 : sorted_flag, out); write_var_int(cast_set(events_list.size()), out); for (const auto& evt : events_list) { // Use fixed-size binary write for timestamp (8 bytes) and event_idx (1 byte). @@ -235,9 +248,13 @@ struct WindowFunnelStateV2 { read_var_int(tmp, in); sorted = (tmp != 0); + // Legacy states use 0/1 and retain their configuration even without events. + initialized = tmp == 2 || window != WINDOW_UNSET || + window_funnel_mode != WindowFunnelMode::INVALID; Int64 size = 0; read_var_int(size, in); + initialized |= size != 0; events_list.clear(); events_list.resize(size); for (Int64 i = 0; i < size; ++i) { diff --git a/be/test/exprs/aggregate/agg_state_parameters_test.cpp b/be/test/exprs/aggregate/agg_state_parameters_test.cpp index 3308e57c041fea..85ee3dd8c2fd08 100644 --- a/be/test/exprs/aggregate/agg_state_parameters_test.cpp +++ b/be/test/exprs/aggregate/agg_state_parameters_test.cpp @@ -77,6 +77,9 @@ class StateParameterChecks { expect_incompatible([&] { _function->deserialize_and_merge_from_column(destination, *serialized, _arena); }); + auto* fresh_direct_destination = create(); + _function->merge(fresh_direct_destination, create(first), _arena); + expect_incompatible([&] { _function->merge(fresh_direct_destination, source, _arena); }); auto* fresh_destination = create(); auto serialized_first = serialize(create(first)); _function->deserialize_and_merge_from_column(fresh_destination, *serialized_first, _arena); @@ -85,8 +88,7 @@ class StateParameterChecks { }); } - void check_sequence_merge(const Arguments& initial, const Arguments& incoming, - bool serialized) { + void check_merge_result(const Arguments& initial, const Arguments& incoming, bool serialized) { auto* destination = create(); auto* source = create(); auto* expected = create(); @@ -113,6 +115,10 @@ class StateParameterChecks { auto* source = create(); EXPECT_NO_THROW(_function->merge(destination, source, _arena)); _function->reset(destination); + auto* configured = create(second); + auto serialized_reset = serialize(destination); + _function->deserialize_and_merge_from_column(configured, *serialized_reset, _arena); + EXPECT_TRUE(ColumnHelper::column_equal(result(configured), result(create(second)))); auto serialized = serialize(create(second)); EXPECT_NO_THROW( _function->deserialize_and_merge_from_column(destination, *serialized, _arena)); @@ -363,10 +369,54 @@ TEST(AggregateStateParametersTest, SequenceEventlessParameters) { for (const auto& initial : {Arguments {}, eventless, contributing}) { for (const auto& incoming : {Arguments {}, eventless, contributing}) { for (bool serialized : {false, true}) { - checks.check_sequence_merge(initial, incoming, serialized); + checks.check_merge_result(initial, incoming, serialized); } } } } } + +TEST(AggregateStateParametersTest, WindowFunnelEventlessParameters) { + DateV2Value time; + time.unchecked_set_time(2024, 1, 1, 0, 0, 0, 0); + auto timestamp = argument(time); + auto yes = argument(1); + auto no = argument(0); + for (const auto& name : {"window_funnel_v1", "window_funnel_v2"}) { + SCOPED_TRACE(name); + Arguments eventless {argument(0), argument("default"), + timestamp, no, no}; + for (const auto& event : {yes, no}) { + check_parameters(name, eventless, + {argument(3), argument("default"), + timestamp, event, no}); + check_parameters(name, eventless, + {argument(0), argument("fixed"), + timestamp, event, no}); + } + DataTypes types; + for (const auto& arg : eventless) { + types.push_back(arg.type); + } + auto function = AggregateFunctionSimpleFactory::instance().get( + name, types, nullptr, false, BeExecVersionManager::get_newest_version()); + ASSERT_NE(function, nullptr); + function->set_version(BeExecVersionManager::get_newest_version()); + StateParameterChecks checks(function); + Arguments contributing {argument(0), argument("default"), + timestamp, yes, no}; + for (const auto& initial : {Arguments {}, eventless, contributing}) { + for (const auto& incoming : {Arguments {}, eventless, contributing}) { + for (bool serialized : {false, true}) { + checks.check_merge_result(initial, incoming, serialized); + } + } + } + } + // Even arguments that equal the fresh-state sentinels establish configuration. + check_parameters( + "window_funnel_v2", + {argument(-1), argument("invalid"), timestamp, no, no}, + {argument(0), argument("default"), timestamp, no, no}); +} } // namespace doris diff --git a/be/test/exprs/aggregate/vec_window_funnel_v2_test.cpp b/be/test/exprs/aggregate/vec_window_funnel_v2_test.cpp index 9b907d153d1755..9cf8917a45490f 100644 --- a/be/test/exprs/aggregate/vec_window_funnel_v2_test.cpp +++ b/be/test/exprs/aggregate/vec_window_funnel_v2_test.cpp @@ -118,6 +118,94 @@ TEST_F(VWindowFunnelV2Test, testEmpty) { agg_function->destroy(place2); } +namespace { +void check_legacy_eventless_configuration(int64_t window, WindowFunnelMode mode) { + ColumnString buffer; + VectorBufferWriter writer(buffer); + // Original V2 header: event count, window, mode, sorted, payload size. + write_var_int(2, writer); + write_var_int(window, writer); + write_var_int(static_cast(mode), writer); + write_var_int(1, writer); + write_var_int(0, writer); + writer.commit(); + + VectorBufferReader reader(buffer.get_data_at(0)); + WindowFunnelStateV2 state; + state.read(reader); + EXPECT_EQ(state.initialized, window != -1 || mode != WindowFunnelMode::INVALID); + EXPECT_EQ(state.window, window); + EXPECT_EQ(state.window_funnel_mode, mode); + EXPECT_EQ(state.event_count, 2); + EXPECT_TRUE(state.events_list.empty()); + + WindowFunnelStateV2 destination; + destination.merge(state); + EXPECT_EQ(destination.initialized, state.initialized); + EXPECT_EQ(destination.window, window); + EXPECT_EQ(destination.window_funnel_mode, mode); +} + +void check_configured_eventless_encoding(bool reset) { + auto timestamp = ColumnDateTimeV2::create(); + DateV2Value time; + time.unchecked_set_time(2024, 1, 1, 0, 0, 0, 0); + timestamp->insert_value(time); + auto event = ColumnUInt8::create(); + event->insert_value(0); + const IColumn* columns[] = {nullptr, nullptr, timestamp.get(), event.get()}; + + WindowFunnelStateV2 state(1); + // These values collide with fresh-state sentinels, so the serialized tag is necessary. + state.add(columns, 0, -1, WindowFunnelMode::INVALID); + if (reset) { + state.reset(); + } + ColumnString buffer; + VectorBufferWriter writer(buffer); + state.write(writer); + write_var_int(42, writer); + writer.commit(); + + VectorBufferReader legacy_reader(buffer.get_data_at(0)); + Int64 value; + read_var_int(value, legacy_reader); + EXPECT_EQ(value, 1); + read_var_int(value, legacy_reader); + EXPECT_EQ(value, -1); + read_var_int(value, legacy_reader); + EXPECT_EQ(value, static_cast(WindowFunnelMode::INVALID)); + read_var_int(value, legacy_reader); + EXPECT_TRUE(value != 0); // Legacy readers decode the sorted flag this way. + read_var_int(value, legacy_reader); + EXPECT_EQ(value, 0); + read_var_int(value, legacy_reader); + EXPECT_EQ(value, 42); + + VectorBufferReader reader(buffer.get_data_at(0)); + WindowFunnelStateV2 restored; + restored.read(reader); + EXPECT_EQ(restored.initialized, !reset); + EXPECT_TRUE(restored.events_list.empty()); + EXPECT_TRUE(restored.sorted); + read_var_int(value, reader); + EXPECT_EQ(value, 42); +} +} // namespace + +TEST(VWindowFunnelV2SerializationTest, LegacyEventlessConfiguration) { + for (int64_t window : {-1, 0, 3}) { + for (auto mode : {WindowFunnelMode::INVALID, WindowFunnelMode::DEFAULT}) { + check_legacy_eventless_configuration(window, mode); + } + } +} + +TEST(VWindowFunnelV2SerializationTest, ConfiguredEventlessEncodingKeepsLegacyLayout) { + check_configured_eventless_encoding(false); + check_configured_eventless_encoding(true); +} + TEST_F(VWindowFunnelV2Test, testSerialize) { const int NUM_CONDS = 4; auto column_mode = ColumnString::create(); diff --git a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy index e5182620eee5f6..92207fad5bc848 100644 --- a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy +++ b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy @@ -108,6 +108,13 @@ suite("test_agg_state_parameters") { "3, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), true, false"]) cases.add([function, "1, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), true, false", "1, cast('fixed' as string), non_nullable(cast('2024-01-01' as datetime)), true, false"]) + // V2 stores no events for an all-false row, but its configuration still participates. + for (def event : ["true", "false"]) { + cases.add([function, "0, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), false, false", + "3, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), ${event}, false"]) + cases.add([function, "0, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), false, false", + "0, cast('fixed' as string), non_nullable(cast('2024-01-01' as datetime)), ${event}, false"]) + } } for (def entry : cases) { From 3d605171549759cdfba0321118c21e4efef15e1c Mon Sep 17 00:00:00 2001 From: happenlee Date: Sat, 12 Sep 2026 02:48:52 +0800 Subject: [PATCH 11/16] [doc](be) Clarify empty reservoir state merge semantics ### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: Clarify that percentile_reservoir states without non-NaN samples are intentionally non-contributing. Their recorded quantile levels are ignored in either merge order, including after serialization. Parameter compatibility applies only when both states contain non-NaN samples. Add a source comment documenting this existing behavior. ### Release note None ### Check List (For Author) - Test: No need to test (comment-only change); clang-format 16.0.6, build hygiene, and git diff --check passed - Behavior changed: No - Does this need documentation: No (the source comment documents the intended contract) --- .../exprs/aggregate/aggregate_function_percentile_reservoir.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h b/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h index 2c2bd0d3e8fc98..3137b248e720f3 100644 --- a/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h +++ b/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h @@ -45,6 +45,9 @@ struct QuantileReservoirSampler { } void merge(const QuantileReservoirSampler& rhs) { + // States without non-NaN samples are non-contributing, even if add() recorded a level. + // Their levels are intentionally ignored in either merge order, including after + // serialization. Only states with non-NaN samples must have matching levels. if (rhs.data.empty()) { return; } From 1ba9b391e9f2490df06e63f0768e8cebf8dc0bca Mon Sep 17 00:00:00 2001 From: happenlee Date: Sat, 12 Sep 2026 03:51:19 +0800 Subject: [PATCH 12/16] [fix](be) Unify configured-empty aggregate state semantics ### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: Reservoir states built from all-NaN samples, EMA states with zero half decay, and limited collect states with negative limits could bypass parameter checks or lose values during merging. TopN with zero capacity could treat an empty counter map as full and adjust real counters by UINT64_MAX. Distinguish initialization from retained payload across these aggregates. Preserve every valid configuration, check incompatible initialized states before empty-payload handling, and clear configuration on reset. Reuse a shared collect-limit marker across all specializations, retain serialized field layouts, and cover direct/serialized merges and empty analytic frames. Historical trial AggState encodings remain outside compatibility scope. A separately reproduced AggState nullability/type mismatch remains for follow-up discussion. ### Release note Configured aggregate states now retain parameters even without effective data. Negative collect limits preserve unlimited collection when compatible states are merged. Zero EMA half decay remains valid but conflicts with a different configured decay. Empty TopN payloads no longer alter compatible counters at zero capacity. Reset states remain identities. ### Check List (For Author) - Test: BE ASAN build, clang-format 16.0.6 and header hygiene passed. clang-tidy has no changed-line diagnostics; existing diagnostics prevent a clean full run. - Regression test / Unit Test: 79 BE ASAN tests and 7 SQL regression suites passed. Generated the new .out with the repository runner and verified it in a normal run. - Behavior changed: Yes, as described above - Does this need documentation: Yes, included aggregate-state and EMA semantics notes --- .../aggregate/aggregate_function_collect.h | 69 +++++--- .../exprs/aggregate/aggregate_function_ema.h | 28 +++- .../aggregate_function_percentile_reservoir.h | 14 +- .../exprs/aggregate/aggregate_function_topn.h | 10 ++ .../aggregate_function_window_funnel.h | 6 +- be/src/util/reservoir_sampler.h | 2 - .../aggregate/agg_state_parameters_test.cpp | 149 ++++++++++++++++++ docs/aggregate-state-parameters.md | 37 +++++ docs/exponential-moving-average.md | 25 +-- .../agg_state/test_agg_state_parameters.out | 37 +++++ .../test_agg_state_parameters.groovy | 105 ++++++++++++ 11 files changed, 429 insertions(+), 53 deletions(-) create mode 100644 docs/aggregate-state-parameters.md create mode 100644 regression-test/data/datatype_p0/agg_state/test_agg_state_parameters.out diff --git a/be/src/exprs/aggregate/aggregate_function_collect.h b/be/src/exprs/aggregate/aggregate_function_collect.h index 7c25b75c04fa5b..427e3b9144bb88 100644 --- a/be/src/exprs/aggregate/aggregate_function_collect.h +++ b/be/src/exprs/aggregate/aggregate_function_collect.h @@ -46,15 +46,24 @@ #include "util/var_int.h" namespace doris { +struct AggregateFunctionCollectLimitData { + // Limits are Int32 inputs. Keep all of them, including negative limits, distinct from + // the fresh/reset marker while retaining the existing Int64 serialized field. + static constexpr Int64 UNINITIALIZED_MAX_SIZE = + static_cast(std::numeric_limits::min()) - 1; + Int64 max_size = UNINITIALIZED_MAX_SIZE; + + bool is_initialized() const { return max_size != UNINITIALIZED_MAX_SIZE; } +}; + template -struct AggregateFunctionCollectSetData { +struct AggregateFunctionCollectSetData : AggregateFunctionCollectLimitData { static constexpr PrimitiveType PType = T; using ElementType = typename PrimitiveTypeTraits::CppType; using ColVecType = typename PrimitiveTypeTraits::ColumnType; using SelfType = AggregateFunctionCollectSetData; using Set = doris::flat_hash_set; Set data_set; - Int64 max_size = -1; AggregateFunctionCollectSetData(const DataTypes& argument_types) {} @@ -67,7 +76,7 @@ struct AggregateFunctionCollectSetData { void merge(const SelfType& rhs) { if constexpr (HasLimit) { - if (max_size == -1) { + if (!is_initialized()) { max_size = rhs.max_size; } @@ -111,19 +120,21 @@ struct AggregateFunctionCollectSetData { } } - void reset() { data_set.clear(); } + void reset() { + data_set.clear(); + max_size = UNINITIALIZED_MAX_SIZE; + } }; template requires(is_string_type(T)) -struct AggregateFunctionCollectSetData { +struct AggregateFunctionCollectSetData : AggregateFunctionCollectLimitData { static constexpr PrimitiveType PType = T; using ElementType = StringRef; using ColVecType = ColumnString; using SelfType = AggregateFunctionCollectSetData; using Set = doris::flat_hash_set; Set data_set; - Int64 max_size = -1; AggregateFunctionCollectSetData(const DataTypes& argument_types) {} @@ -136,7 +147,7 @@ struct AggregateFunctionCollectSetData { } void merge(const SelfType& rhs, Arena& arena) { - if (max_size == -1) { + if (!is_initialized()) { max_size = rhs.max_size; } @@ -179,17 +190,19 @@ struct AggregateFunctionCollectSetData { } } - void reset() { data_set.clear(); } + void reset() { + data_set.clear(); + max_size = UNINITIALIZED_MAX_SIZE; + } }; template -struct AggregateFunctionCollectListData { +struct AggregateFunctionCollectListData : AggregateFunctionCollectLimitData { static constexpr PrimitiveType PType = T; using ElementType = typename PrimitiveTypeTraits::CppType; using ColVecType = typename PrimitiveTypeTraits::ColumnType; using SelfType = AggregateFunctionCollectListData; PaddedPODArray data; - Int64 max_size = -1; AggregateFunctionCollectListData(const DataTypes& argument_types) {} @@ -203,7 +216,7 @@ struct AggregateFunctionCollectListData { void merge(const SelfType& rhs) { if constexpr (HasLimit) { - if (max_size == -1) { + if (!is_initialized()) { max_size = rhs.max_size; } for (auto& rhs_elem : rhs.data) { @@ -231,7 +244,10 @@ struct AggregateFunctionCollectListData { read_var_int(max_size, buf); } - void reset() { data.clear(); } + void reset() { + data.clear(); + max_size = UNINITIALIZED_MAX_SIZE; + } void insert_result_into(IColumn& to) const { auto& vec = assert_cast(to).get_data(); @@ -243,12 +259,11 @@ struct AggregateFunctionCollectListData { template requires(is_string_type(T)) -struct AggregateFunctionCollectListData { +struct AggregateFunctionCollectListData : AggregateFunctionCollectLimitData { static constexpr PrimitiveType PType = T; using ElementType = StringRef; using ColVecType = ColumnString; MutableColumnPtr data; - Int64 max_size = -1; AggregateFunctionCollectListData(const DataTypes& argument_types) { data = ColVecType::create(); @@ -260,7 +275,7 @@ struct AggregateFunctionCollectListData { void merge(const AggregateFunctionCollectListData& rhs) { if constexpr (HasLimit) { - if (max_size == -1) { + if (!is_initialized()) { max_size = rhs.max_size; } @@ -297,7 +312,10 @@ struct AggregateFunctionCollectListData { read_var_int(max_size, buf); } - void reset() { data->clear(); } + void reset() { + data->clear(); + max_size = UNINITIALIZED_MAX_SIZE; + } void insert_result_into(IColumn& to) const { auto& to_str = assert_cast(to); @@ -308,13 +326,12 @@ struct AggregateFunctionCollectListData { template requires(!is_string_type(T) && !is_int_or_bool(T) && !is_float_or_double(T) && !is_decimal(T) && !is_date_type(T) && !is_timestamp_ns_type(T) && !is_ip(T) && !is_timestamptz_type(T)) -struct AggregateFunctionCollectListData { +struct AggregateFunctionCollectListData : AggregateFunctionCollectLimitData { static constexpr PrimitiveType PType = T; using ElementType = StringRef; using Self = AggregateFunctionCollectListData; DataTypeSerDeSPtr serde; // for complex serialize && deserialize from multi BE MutableColumnPtr column_data; - Int64 max_size = -1; AggregateFunctionCollectListData(const DataTypes& argument_types) { DataTypePtr column_type = argument_types[0]; @@ -328,7 +345,7 @@ struct AggregateFunctionCollectListData { void merge(const AggregateFunctionCollectListData& rhs) { if constexpr (HasLimit) { - if (max_size == -1) { + if (!is_initialized()) { max_size = rhs.max_size; } @@ -388,7 +405,10 @@ struct AggregateFunctionCollectListData { read_var_int(max_size, buf); } - void reset() { column_data->clear(); } + void reset() { + column_data->clear(); + max_size = UNINITIALIZED_MAX_SIZE; + } void insert_result_into(IColumn& to) const { to.insert_range_from(*column_data, 0, size()); } }; @@ -424,10 +444,9 @@ class AggregateFunctionCollect final Arena& arena) const override { auto& data = this->data(place); if constexpr (HasLimit) { - if (data.max_size == -1) { + if (!data.is_initialized()) { data.max_size = - (UInt64)assert_cast( - columns[1]) + assert_cast(columns[1]) ->get_element(row_num); } if (data.size() >= data.max_size) { @@ -446,10 +465,10 @@ class AggregateFunctionCollect final auto& data = this->data(place); const auto& rhs_data = this->data(rhs); if constexpr (HasLimit) { - if (rhs_data.max_size < 0) { + if (!rhs_data.is_initialized()) { return; } - if (data.max_size != -1) { + if (data.is_initialized()) { if (UNLIKELY(data.max_size != rhs_data.max_size)) { throw Exception(ErrorCode::INVALID_ARGUMENT, "{} aggregate states have incompatible limits: {} vs {}", diff --git a/be/src/exprs/aggregate/aggregate_function_ema.h b/be/src/exprs/aggregate/aggregate_function_ema.h index 972390c8ee3314..2d567d93275023 100644 --- a/be/src/exprs/aggregate/aggregate_function_ema.h +++ b/be/src/exprs/aggregate/aggregate_function_ema.h @@ -21,6 +21,7 @@ #pragma once #include +#include #include #include "common/exception.h" @@ -58,14 +59,15 @@ class IColumn; * - timeunit: numeric time index (not raw timestamp; use intDiv if needed) * Returns DOUBLE. * - * A zero half_decay returns 0 and is also the empty-state marker. Serialized - * zero-half-decay states do not contribute to _merge/_union and do not trigger - * a half-decay mismatch. Compatibility checks apply to nonzero half decays. + * A zero half_decay returns 0 but remains an initialized configuration. Only + * fresh/reset states are identities; all initialized states must have matching + * half decays when merged. */ struct ExponentialMovingAverageData { double value = 0.0; double time = 0.0; double half_decay = 0.0; + bool initialized = false; static double scale(double time_passed, double hd) { return std::exp2(-time_passed / hd); } @@ -73,6 +75,7 @@ struct ExponentialMovingAverageData { void add(double new_value, double current_time, double hd) { half_decay = hd; + initialized = true; ExponentialMovingAverageData other; other.value = new_value; other.time = current_time; @@ -91,12 +94,14 @@ struct ExponentialMovingAverageData { } void merge(const ExponentialMovingAverageData& rhs) { - if (rhs.half_decay == 0.0) { + if (!rhs.initialized) { return; } - if (half_decay == 0.0) { - half_decay = rhs.half_decay; - } else if (UNLIKELY(half_decay != rhs.half_decay)) { + if (!initialized) { + *this = rhs; + return; + } + if (UNLIKELY(half_decay != rhs.half_decay)) { throw Exception( ErrorCode::INVALID_ARGUMENT, "exponential_moving_average aggregate states have incompatible half decay"); @@ -116,13 +121,19 @@ struct ExponentialMovingAverageData { check_half_decay(); buf.write_binary(value); buf.write_binary(time); - buf.write_binary(half_decay); + // NaN is rejected for configured states, so it can encode initialization without + // adding a field or colliding with the valid zero half-decay configuration. + buf.write_binary(initialized ? half_decay : std::numeric_limits::quiet_NaN()); } void read(BufferReadable& buf) { buf.read_binary(value); buf.read_binary(time); buf.read_binary(half_decay); + initialized = !std::isnan(half_decay); + if (!initialized) { + half_decay = 0.0; + } } void check_half_decay() const { @@ -136,6 +147,7 @@ struct ExponentialMovingAverageData { value = 0.0; time = 0.0; half_decay = 0.0; + initialized = false; } }; diff --git a/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h b/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h index 3137b248e720f3..4240f24bdb758c 100644 --- a/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h +++ b/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h @@ -45,13 +45,11 @@ struct QuantileReservoirSampler { } void merge(const QuantileReservoirSampler& rhs) { - // States without non-NaN samples are non-contributing, even if add() recorded a level. - // Their levels are intentionally ignored in either merge order, including after - // serialization. Only states with non-NaN samples must have matching levels. - if (rhs.data.empty()) { + // NaN samples do not erase the quantile established by a non-null input row. + if (rhs.level == INIT_QUANTILE) { return; } - if (data.empty()) { + if (level == INIT_QUANTILE) { level = rhs.level; } else if (UNLIKELY(level != rhs.level)) { throw Exception(ErrorCode::INVALID_ARGUMENT, @@ -61,7 +59,7 @@ struct QuantileReservoirSampler { } void reset() { - level = 0.0; + level = INIT_QUANTILE; data.clear(); } @@ -82,7 +80,9 @@ struct QuantileReservoirSampler { } private: - double level = 0.0; + // Valid quantiles include zero, so use an out-of-range value for fresh/reset states. + static constexpr double INIT_QUANTILE = -1.0; + double level = INIT_QUANTILE; ReservoirSampler data; }; diff --git a/be/src/exprs/aggregate/aggregate_function_topn.h b/be/src/exprs/aggregate/aggregate_function_topn.h index 8e35276186d233..8cfcde8d422c1f 100644 --- a/be/src/exprs/aggregate/aggregate_function_topn.h +++ b/be/src/exprs/aggregate/aggregate_function_topn.h @@ -96,6 +96,16 @@ struct AggregateFunctionTopNData { top_num, capacity, rhs.top_num, rhs.capacity); } + // Empty payloads still carry configuration. Check it above, then avoid treating + // an empty zero-capacity map as full and adding UINT64_MAX to real counters. + if (rhs.counter_map.empty()) { + return; + } + if (counter_map.empty()) { + counter_map = rhs.counter_map; + return; + } + bool lhs_full = (counter_map.size() >= capacity); bool rhs_full = (rhs.counter_map.size() >= capacity); diff --git a/be/src/exprs/aggregate/aggregate_function_window_funnel.h b/be/src/exprs/aggregate/aggregate_function_window_funnel.h index 28ffa997d5200f..cc65ce79fa478d 100644 --- a/be/src/exprs/aggregate/aggregate_function_window_funnel.h +++ b/be/src/exprs/aggregate/aggregate_function_window_funnel.h @@ -120,7 +120,11 @@ struct WindowFunnelState { events_list.event_columns_data.resize(event_count); } - void reset() { events_list.clear(); } + void reset() { + events_list.clear(); + window = 0; + window_funnel_mode = WindowFunnelMode::INVALID; + } void add(const IColumn** arg_columns, ssize_t row_num, int64_t win, WindowFunnelMode mode) { window = win; diff --git a/be/src/util/reservoir_sampler.h b/be/src/util/reservoir_sampler.h index 79b456c694b1a8..a54a2cf89b0fea 100644 --- a/be/src/util/reservoir_sampler.h +++ b/be/src/util/reservoir_sampler.h @@ -349,8 +349,6 @@ class ReservoirSampler { return samples[left_index] * left_coef + samples[right_index] * right_coef; } - bool empty() const { return total_values == 0; } - void merge(const ReservoirSampler& b) { if (sample_count != b.sample_count) { throw doris::Exception(ErrorCode::INTERNAL_ERROR, diff --git a/be/test/exprs/aggregate/agg_state_parameters_test.cpp b/be/test/exprs/aggregate/agg_state_parameters_test.cpp index 85ee3dd8c2fd08..5457b2488c54c5 100644 --- a/be/test/exprs/aggregate/agg_state_parameters_test.cpp +++ b/be/test/exprs/aggregate/agg_state_parameters_test.cpp @@ -111,6 +111,10 @@ class StateParameterChecks { } void check_empty_and_reset(const Arguments& first, const Arguments& second) { + check_merge_result({}, first, false); + check_merge_result({}, first, true); + check_merge_result(first, {}, false); + check_merge_result(first, {}, true); auto* destination = create(first); auto* source = create(); EXPECT_NO_THROW(_function->merge(destination, source, _arena)); @@ -125,6 +129,34 @@ class StateParameterChecks { EXPECT_TRUE(ColumnHelper::column_equal(result(destination), result(create(second)))); } + void check_mismatch_and_reset(const Arguments& first, const Arguments& second) { + check_mismatch(first, second); + auto* reset_state = create(first); + _function->reset(reset_state); + auto* configured = create(second); + auto serialized = serialize(reset_state); + EXPECT_NO_THROW( + _function->deserialize_and_merge_from_column(configured, *serialized, _arena)); + EXPECT_TRUE(ColumnHelper::column_equal(result(configured), result(create(second)))); + EXPECT_NO_THROW(_function->merge(reset_state, create(second), _arena)); + EXPECT_TRUE(ColumnHelper::column_equal(result(reset_state), result(create(second)))); + } + + void check_configured_empty_payload(const Arguments& arguments) { + // TopN with zero capacity keeps counters in memory but serializes no counters. + // A decoded state must preserve configuration without changing compatible counters. + auto serialized = serialize(create(arguments)); + auto* configured_empty = create(); + _function->deserialize_and_merge_from_column(configured_empty, *serialized, _arena); + auto* populated = create(arguments); + EXPECT_NO_THROW( + _function->deserialize_and_merge_from_column(populated, *serialized, _arena)); + EXPECT_TRUE(ColumnHelper::column_equal(result(populated), result(create(arguments)))); + EXPECT_NO_THROW(_function->merge(configured_empty, create(arguments), _arena)); + EXPECT_TRUE( + ColumnHelper::column_equal(result(configured_empty), result(create(arguments)))); + } + void check_compatible(const Arguments& arguments) { auto* destination = create(); auto serialized = serialize(create(arguments)); @@ -214,6 +246,27 @@ void check_parameters(const std::string& name, const Arguments& first, const Arg checks.check_compatible(lhs_args); } } + +void check_compatible_states(const std::string& name, const Arguments& first, + const Arguments& second) { + SCOPED_TRACE(name); + DataTypes types; + for (const auto& arg : first) { + types.push_back(arg.type); + } + auto function = AggregateFunctionSimpleFactory::instance().get( + name, types, nullptr, false, BeExecVersionManager::get_newest_version()); + ASSERT_NE(function, nullptr); + function->set_version(BeExecVersionManager::get_newest_version()); + StateParameterChecks checks(function); + for (const auto& initial : {Arguments {}, first, second}) { + for (const auto& incoming : {Arguments {}, first, second}) { + for (bool serialized : {false, true}) { + checks.check_merge_result(initial, incoming, serialized); + } + } + } +} } // namespace TEST(AggregateStateParametersTest, TopN) { @@ -245,6 +298,36 @@ TEST(AggregateStateParametersTest, Histogram) { {value, argument(3)}); } +TEST(AggregateStateParametersTest, TopNZeroCapacityParameters) { + for (const auto& name : {"topn", "topn_array", "topn_weighted"}) { + SCOPED_TRACE(name); + Arguments zero_capacity {argument("a")}; + if (std::string(name) == "topn_weighted") { + zero_capacity.push_back(argument(1)); + } + zero_capacity.push_back(argument(1)); + zero_capacity.push_back(argument(0)); + auto positive_capacity = zero_capacity; + positive_capacity.back() = argument(2); + DataTypes types; + for (const auto& arg : zero_capacity) { + types.push_back(arg.type); + } + auto function = AggregateFunctionSimpleFactory::instance().get( + name, types, nullptr, false, BeExecVersionManager::get_newest_version()); + ASSERT_NE(function, nullptr); + StateParameterChecks checks(function); + // A zero capacity serializes no retained elements but still establishes N/capacity. + checks.check_mismatch(zero_capacity, positive_capacity); + checks.check_mismatch(positive_capacity, zero_capacity); + checks.check_mismatch_and_reset(zero_capacity, positive_capacity); + checks.check_merge_result({}, zero_capacity, false); + checks.check_merge_result(zero_capacity, {}, false); + checks.check_merge_result(zero_capacity, {}, true); + checks.check_configured_empty_payload(zero_capacity); + } +} + TEST(AggregateStateParametersTest, Percentiles) { auto value = argument(7); for (const auto& name : @@ -271,6 +354,31 @@ TEST(AggregateStateParametersTest, Percentiles) { argument(4096)}); } +TEST(AggregateStateParametersTest, PercentileConfiguredEmptyParameters) { + const auto nan = argument(std::numeric_limits::quiet_NaN()); + const auto value = argument(7); + for (const auto& name : {"percentile_v2", "percentile_approx", "percentile_reservoir"}) { + for (double level : {0.0, 0.25, 1.0}) { + const auto quantile = argument(level); + for (const auto& sample : {nan, value}) { + check_parameters(name, {nan, quantile}, {sample, argument(0.75)}); + } + check_compatible_states(name, {nan, quantile}, {value, quantile}); + } + } + for (const auto& name : {"percentile_array_v2", "percentile_approx_array"}) { + check_parameters(name, {nan, quantiles({0.25})}, {value, quantiles({0.75})}); + check_parameters(name, {nan, quantiles({0.25})}, {nan, quantiles({0.75})}); + check_compatible_states(name, {nan, quantiles({0.25})}, {value, quantiles({0.25})}); + } + check_parameters("percentile_approx_weighted", + {value, argument(0), argument(0.25)}, + {value, argument(1), argument(0.75)}); + check_compatible_states("percentile_approx_weighted", + {value, argument(0), argument(0.25)}, + {value, argument(1), argument(0.25)}); +} + TEST(AggregateStateParametersTest, CollectAndConcat) { for (const auto& name : {"collect_list", "collect_set"}) { for (const auto& value : {argument(7), argument("a")}) { @@ -285,6 +393,33 @@ TEST(AggregateStateParametersTest, CollectAndConcat) { {value, argument(";")}); } +TEST(AggregateStateParametersTest, CollectZeroAndNegativeLimits) { + for (const auto& name : {"collect_list", "collect_set"}) { + for (const auto& value : {argument(7), argument("a")}) { + for (int limit : {std::numeric_limits::min(), -2, -1, 0, + std::numeric_limits::max()}) { + check_parameters(name, {value, argument(limit)}, + {value, argument(1)}); + } + check_parameters(name, {value, argument(-1)}, + {value, argument(-2)}); + } + } + for (int limit : {-2, -1, 0}) { + check_parameters("collect_list", {quantiles({0.5}), argument(limit)}, + {quantiles({0.5}), argument(1)}); + } +} + +TEST(AggregateStateParametersTest, GroupConcatEmptyStrings) { + auto empty = argument(""); + auto comma = argument(","); + auto semicolon = argument(";"); + check_parameters("group_concat", {empty, comma}, {empty, semicolon}); + check_parameters("group_concat", {empty, comma}, {argument("a"), semicolon}); + check_compatible_states("group_concat", {empty, comma}, {argument("a"), comma}); +} + TEST(AggregateStateParametersTest, IntersectCount) { auto bitmap_column = ColumnBitmap::create(); bitmap_column->insert_value(BitmapValue {uint64_t(1)}); @@ -295,6 +430,14 @@ TEST(AggregateStateParametersTest, IntersectCount) { auto text = argument("a"); check_parameters("intersect_count", {bitmap, text, argument("a")}, {bitmap, text, argument("b")}); + auto empty_column = ColumnBitmap::create(); + empty_column->insert_value(BitmapValue {}); + ColumnWithTypeAndName empty_bitmap {std::move(empty_column), std::make_shared(), + ""}; + check_parameters("intersect_count", {empty_bitmap, value, argument(1)}, + {bitmap, value, argument(2)}); + check_parameters("intersect_count", {empty_bitmap, text, argument("a")}, + {empty_bitmap, text, argument("b")}); } TEST(AggregateStateParametersTest, ExponentialMovingAverage) { @@ -302,6 +445,12 @@ TEST(AggregateStateParametersTest, ExponentialMovingAverage) { auto time = argument(1); check_parameters("exponential_moving_average", {argument(1), value, time}, {argument(2), value, time}); + check_parameters("exponential_moving_average", {argument(0), value, time}, + {argument(1), value, time}); + auto zero = argument(0); + check_parameters("exponential_moving_average", {zero, zero, zero}, + {argument(1), value, time}); + check_compatible_states("exponential_moving_average", {zero, zero, zero}, {zero, value, time}); } TEST(AggregateStateParametersTest, ExponentialMovingAverageNaNOutputs) { diff --git a/docs/aggregate-state-parameters.md b/docs/aggregate-state-parameters.md new file mode 100644 index 00000000000000..9d78ccd9184130 --- /dev/null +++ b/docs/aggregate-state-parameters.md @@ -0,0 +1,37 @@ +# Aggregate-state parameter compatibility + +Parameterized aggregate states distinguish initialization from retained data: + +- A fresh state has no configuration. It is an identity during merge. +- A non-null input establishes configuration even when it retains no effective + data. Merging two initialized states requires compatible parameter values. +- `reset()` discards both data and configuration. The resulting state is an + identity, including after serialization and deserialization. + +These rules apply in both merge orders and to `_merge` and `_union`. Examples of +initialized states include a percentile reservoir with only NaN samples, an exact +percentile V2 with only NaN samples, a zero-weight approximate percentile, a sequence +or funnel with all-false events, a zero-limit collection, an empty-string +`group_concat`, an empty bitmap intersection, and a zero-half-decay moving average. +TopN retains its N/capacity configuration even when a zero capacity causes its +serialized payload to contain no elements. After configuration checks, empty +TopN payloads do not change compatible counters; this also prevents zero-capacity +empty maps from applying an invalid full-map count adjustment. + +Negative collection limits retain the existing unlimited-collection behavior; +they are configurations and must match when states are merged. A zero EMA +half-decay still produces zero, but it is a configuration rather than an identity. +Invalid parameters remain subject to each function's existing validation. + +Existing initialization flags are reused where available. Reservoir reserves +quantile `-1` for fresh/reset states, outside the valid interval `[0, 1]`. Limited +collection states reserve one below the minimum Int32 value, outside their +parameter domain and within the serialized varint range. EMA tracks initialization +independently in memory and reserves a serialized NaN half-decay for fresh/reset +states; configured NaN half-decays cannot be serialized. +The existing serialized field layouts are retained. + +Historical states from the trial AggState implementation are outside this change's +compatibility scope. In particular, old reset states may contain stale configuration +values that cannot always be distinguished from initialized states. The rules above +are guaranteed for states produced by the updated implementation. diff --git a/docs/exponential-moving-average.md b/docs/exponential-moving-average.md index 335eb2e626c2cb..d134fcd964297c 100644 --- a/docs/exponential-moving-average.md +++ b/docs/exponential-moving-average.md @@ -11,24 +11,29 @@ SELECT exponential_moving_average(0, 7, 1); -- 0 ``` -A zero half-decay also marks an empty aggregate state. When combining serialized -states with `exponential_moving_average_merge` or -`exponential_moving_average_union`, states whose half-decay is `0` are ignored. -This applies even when the state was created from non-null input rows. +A non-null input establishes the half-decay configuration, including when it is +`0`. When combining states with `exponential_moving_average_merge` or +`exponential_moving_average_union`, every initialized state participates in +parameter compatibility checks, regardless of its final numeric result. Consequently: -- Combining states with half-decays `0` and `1` uses the contributing state with - half-decay `1`, without a parameter-mismatch error, in either input order. -- Combining contributing states with different nonzero half-decays, such as `1` - and `2`, raises an incompatible-half-decay error. +- Combining initialized states with half-decays `0` and `1` raises an + incompatible-half-decay error in either input order, including after serialization. +- Combining initialized states with different nonzero half-decays, such as `1` + and `2`, also raises an incompatible-half-decay error. - Combining non-null states whose half-decays are all `0` yields `0` when the aggregate is finalized. -This documents the existing zero-half-decay behavior. Zero is not a distinct -contributing configuration for aggregate-state compatibility checks. SQL NULL +Fresh states and states cleared by `reset()` have no configuration. They are +identities during merge and adopt the other state's configuration. SQL NULL handling is unchanged. A NaN half-decay is unsupported. Serializing an aggregate state or finalizing its result raises `exponential_moving_average half decay must not be NaN`. This check applies to the half-decay parameter, not to the input value or the computed result. + +The serialized state retains its three-double layout. Since configured NaN +half-decays are rejected before serialization, a NaN in the serialized half-decay +field is reserved for a fresh/reset state. It is decoded as an uninitialized +state, separately from an initialized state whose half-decay is zero. diff --git a/regression-test/data/datatype_p0/agg_state/test_agg_state_parameters.out b/regression-test/data/datatype_p0/agg_state/test_agg_state_parameters.out new file mode 100644 index 00000000000000..5e03dc7d603597 --- /dev/null +++ b/regression-test/data/datatype_p0/agg_state/test_agg_state_parameters.out @@ -0,0 +1,37 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !reservoir_compatible_nan -- +100 + +-- !ema_compatible_zero -- +0 + +-- !collect_list_negative_limit -- +[7, 8] + +-- !collect_set_negative_limit -- +[7, 8] + +-- !percentile_reservoir_reset_frame -- +100 + +-- !exponential_moving_average_reset_frame -- +3.5 + +-- !collect_list_reset_frame -- +[8] + +-- !collect_set_reset_frame -- +[8] + +-- !histogram_reset_frame -- +{"num_buckets":1,"buckets":[{"lower":"8","upper":"8","ndv":1,"count":1,"pre_sum":0}]} + +-- !topn_reset_frame -- +{"b":1} + +-- !topn_array_reset_frame -- +[2] + +-- !topn_weighted_reset_frame -- +[2] + diff --git a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy index 92207fad5bc848..92b2108783dfac 100644 --- a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy +++ b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy @@ -63,11 +63,14 @@ suite("test_agg_state_parameters") { ["topn", "'a', 1", "'a', 3"], ["topn", "'a', 3, 2", "'a', 3, 5"], ["topn", "'a', 1, 6", "'a', 3, 2"], + ["topn", "'a', 1, 0", "'a', 1, 2"], ["topn_array", "1, 1", "1, 3"], ["topn_array", "1, 3, 2", "1, 3, 5"], ["topn_array", "'a', 1, 6", "'a', 3, 2"], + ["topn_array", "1, 1, 0", "1, 1, 2"], ["topn_weighted", "1, 1, 1", "1, 1, 3"], ["topn_weighted", "1, 1, 3, 2", "1, 1, 3, 5"], + ["topn_weighted", "1, 1, 1, 0", "1, 1, 1, 2"], ["histogram", "7, 1", "7, 3"], ["percentile", "7, 0.25", "7, 0.75"], ["percentile_array", "7, [0.25]", "7, [0.75]"], @@ -78,6 +81,7 @@ suite("test_agg_state_parameters") { ["percentile_approx_array", "7, [0.25]", "7, [0.75]"], ["percentile_approx_array", "7, [0.25], 2048", "7, [0.25], 4096"], ["percentile_approx_weighted", "7, 1, 0.25", "7, 1, 0.75"], + ["percentile_approx_weighted", "7, 0, 0.25", "7, 1, 0.75"], ["percentile_approx_weighted", "7, 1, 0.5, 2048", "7, 1, 0.5, 4096"], ["percentile_reservoir", "7, 0.25", "7, 0.75"], ["collect_list", "7, 1", "7, 3"], @@ -87,13 +91,37 @@ suite("test_agg_state_parameters") { ["collect_set", "'a', 1", "'a', 3"], ["intersect_count", "to_bitmap(1), 1, 1", "to_bitmap(1), 1, 2"], ["intersect_count", "to_bitmap(1), 'a', 'a'", "to_bitmap(1), 'a', 'b'"], + ["intersect_count", "bitmap_empty(), 1, 1", "to_bitmap(1), 1, 2"], ["group_concat", "'a', ','", "'a', ';'"], + ["group_concat", "cast('' as string), ','", "cast('a' as string), ';'"], + ["group_concat", "cast('' as string), ','", "cast('' as string), ';'"], ["exponential_moving_average", "1, 7, 1", "2, 7, 1"], + ["exponential_moving_average", "0, 7, 1", "1, 7, 1"], + ["exponential_moving_average", "0, 0, 0", "1, 7, 1"], ["sequence_match", "'(?1)', non_nullable(cast('2024-01-01' as datetime)), true, false", "'(?2)', non_nullable(cast('2024-01-01' as datetime)), true, false"], ["sequence_count", "'(?1)', non_nullable(cast('2024-01-01' as datetime)), true, false", "'(?2)', non_nullable(cast('2024-01-01' as datetime)), true, false"] ] + // A non-null input establishes configuration even if no sample is retained. + // Keep AggState argument nullability identical across both sides of the UNION. + for (def quantile : ["0.0", "0.25", "1.0"]) { + for (def sample : ["'NaN'", "7"]) { + cases.add(["percentile_reservoir", "non_nullable(cast('NaN' as double)), ${quantile}", + "non_nullable(cast(${sample} as double)), 0.75"]) + } + } + for (def function : ["collect_list", "collect_set"]) { + for (def value : ["7", "'a'"]) { + for (def limit : [-2, -1, 0]) { + cases.add([function, "${value}, ${limit}", "${value}, 1"]) + } + cases.add([function, "${value}, -1", "${value}, -2"]) + } + } + for (def limit : [-2, -1, 0]) { + cases.add(["collect_list", "[7], ${limit}", "[7], 1"]) + } // All-false event rows retain their pattern, including when both states have no events. for (def function : ["sequence_match", "sequence_count"]) { for (def event : ["true", "false"]) { @@ -135,4 +163,81 @@ suite("test_agg_state_parameters") { } } } + + order_qt_reservoir_compatible_nan """ + SELECT percentile_reservoir_merge(s) FROM ( + SELECT percentile_reservoir_state(non_nullable(cast('NaN' as double)), 0.25) s + UNION ALL + SELECT percentile_reservoir_state(cast(100 as double), 0.25) s + ) states + """ + order_qt_ema_compatible_zero """ + SELECT exponential_moving_average_merge(s) FROM ( + SELECT exponential_moving_average_state(0, 0, 0) s + UNION ALL + SELECT exponential_moving_average_state(0, 7, 1) s + ) states + """ + for (def function : ["collect_list", "collect_set"]) { + "order_qt_${function}_negative_limit"(""" + SELECT array_sort(${function}_merge(s)) FROM ( + SELECT ${function}_state(7, -1) s + UNION ALL + SELECT ${function}_state(8, -1) s + ) states + """) + } + + // The last following-row frame is empty after a populated frame. Its reset state + // must accept a different configuration when the serialized result is merged again. + sql "DROP TABLE IF EXISTS test_agg_state_reservoir_reset" + sql """ + CREATE TABLE test_agg_state_reservoir_reset ( + number BIGINT NOT NULL, + s AGG_STATE GENERIC + ) AGGREGATE KEY(number) + DISTRIBUTED BY HASH(number) BUCKETS 1 + PROPERTIES("replication_num" = "1") + """ + sql """ + INSERT INTO test_agg_state_reservoir_reset + SELECT number, percentile_reservoir_state(cast(7 as double), 0.25) + FROM numbers("number" = "2") + """ + order_qt_percentile_reservoir_reset_frame """ + WITH framed AS ( + SELECT number, percentile_reservoir_union(s) OVER ( + ORDER BY number ROWS BETWEEN 1 FOLLOWING AND 1 FOLLOWING + ) s FROM test_agg_state_reservoir_reset + ) + SELECT percentile_reservoir_merge(s) FROM ( + SELECT s FROM framed WHERE number = 1 + UNION ALL + SELECT percentile_reservoir_state(cast(100 as double), 0.75) s + ) states + """ + def resetCases = [ + ["exponential_moving_average", "0, 0, 0", "1, 7, 1"], + ["collect_list", "7, -1", "8, 1"], + ["collect_set", "7, -1", "8, 1"], + ["histogram", "7, 10", "8, 20"], + ["topn", "'a', 1, 0", "'b', 2, 1"], + ["topn_array", "1, 1, 0", "2, 2, 1"], + ["topn_weighted", "1, 1, 1, 0", "2, 1, 2, 1"] + ] + for (def entry : resetCases) { + def function = entry[0] + "order_qt_${function}_reset_frame"(""" + WITH framed AS ( + SELECT number, ${function}_union(${function}_state(${entry[1]})) OVER ( + ORDER BY number ROWS BETWEEN 1 FOLLOWING AND 1 FOLLOWING + ) s FROM numbers("number" = "2") + ) + SELECT ${function}_merge(s) FROM ( + SELECT s FROM framed WHERE number = 1 + UNION ALL + SELECT ${function}_state(${entry[2]}) s + ) states + """) + } } From 822f212f89ec6da1f13f6abe13f4df6a47f77b2b Mon Sep 17 00:00:00 2001 From: happenlee Date: Sat, 12 Sep 2026 13:50:01 +0800 Subject: [PATCH 13/16] [fix](be) Ignore parameters of non-contributing aggregate states ### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: Parameter checks rejected merges of aggregate states without retained counters, elements, samples or events even though these payloads do not contribute to the result. An empty destination could also retain parameters that conflict with subsequent contributing states. Skip non-contributing states before compatibility checks and adopt contributing parameters into empty destinations across TopN, limited collect, percentile, reservoir, sequence and Window Funnel V2 implementations. Remove the V2 initialization flag and sorted-field tag. Retain checks for states that still contribute, including V1 fixed-mode rows, bitmap filter keys, group_concat empty strings and EMA value/time pairs. Preserve empty percentile-array result shapes and negative collection-limit behavior. Historical trial AggState encodings remain outside the compatibility scope. ### Release note Non-contributing aggregate states ignore parameter differences during merging. Empty destinations adopt the contributing state's parameters. Nonempty states continue to reject incompatible parameters. ### Check List (For Author) - Test: Unit Test / Regression test / Manual test - BE ASAN and FE builds passed. - 72 BE ASAN tests passed across 10 suites. - All 7 SQL regression suites passed with zero failures or skips. Generated the 132 new outputs with the repository runner and verified them in a normal run; merge/union and both operand orders agree. - clang-format 16.0.6, header hygiene and git diff --check passed. - clang-tidy reports no diagnostics on changed lines after test-helper refactoring. The full check still reports existing core/types.h NOLINTEND and other pre-existing diagnostics. - Behavior changed: Yes, empty payload parameters no longer reject merges. - Does this need documentation: Yes, updated docs/aggregate-state-parameters.md. --- .../aggregate/aggregate_function_collect.h | 6 +- .../aggregate/aggregate_function_percentile.h | 20 +- .../aggregate_function_percentile_reservoir.h | 11 +- .../aggregate_function_sequence_match.h | 9 +- .../exprs/aggregate/aggregate_function_topn.h | 21 +- .../aggregate_function_window_funnel_v2.h | 53 +-- be/src/util/reservoir_sampler.h | 2 + .../aggregate/agg_state_parameters_test.cpp | 146 +++++-- .../aggregate/vec_window_funnel_v2_test.cpp | 29 +- docs/aggregate-state-parameters.md | 66 +-- .../agg_state/test_agg_state_parameters.out | 396 ++++++++++++++++++ .../test_agg_state_parameters.groovy | 60 ++- 12 files changed, 645 insertions(+), 174 deletions(-) diff --git a/be/src/exprs/aggregate/aggregate_function_collect.h b/be/src/exprs/aggregate/aggregate_function_collect.h index 427e3b9144bb88..8eedf4b555fa02 100644 --- a/be/src/exprs/aggregate/aggregate_function_collect.h +++ b/be/src/exprs/aggregate/aggregate_function_collect.h @@ -465,10 +465,12 @@ class AggregateFunctionCollect final auto& data = this->data(place); const auto& rhs_data = this->data(rhs); if constexpr (HasLimit) { - if (!rhs_data.is_initialized()) { + if (rhs_data.size() == 0) { return; } - if (data.is_initialized()) { + if (data.size() == 0) { + data.max_size = rhs_data.max_size; + } else { if (UNLIKELY(data.max_size != rhs_data.max_size)) { throw Exception(ErrorCode::INVALID_ARGUMENT, "{} aggregate states have incompatible limits: {} vs {}", diff --git a/be/src/exprs/aggregate/aggregate_function_percentile.h b/be/src/exprs/aggregate/aggregate_function_percentile.h index be2ca3f726741f..5bc34c39e35c9e 100644 --- a/be/src/exprs/aggregate/aggregate_function_percentile.h +++ b/be/src/exprs/aggregate/aggregate_function_percentile.h @@ -137,10 +137,10 @@ struct PercentileApproxState { } void merge(const PercentileApproxState& rhs) { - if (!rhs.init_flag) { + if (!rhs.init_flag || rhs.digest->total_size() == 0) { return; } - if (!init_flag) { + if (!init_flag || digest->total_size() == 0) { target_quantile = rhs.target_quantile; compressions = rhs.compressions; digest = TDigest::create_unique(compressions); @@ -408,13 +408,17 @@ struct PercentileApproxArrayState { return; } - if (!init_flag) { - levels.merge(rhs.levels); + // Preserve the result shape when every state is empty, but let contributing + // samples replace parameters recorded by an empty destination. + if (!init_flag || levels.empty() || digest->total_size() == 0) { + levels = rhs.levels; compressions = rhs.compressions; if (!levels.empty()) { digest = TDigest::create_unique(compressions); } init_flag = true; + } else if (rhs.levels.empty() || rhs.digest->total_size() == 0) { + return; } else { if (UNLIKELY(compressions != rhs.compressions || levels.quantiles != rhs.levels.quantiles)) { @@ -639,11 +643,11 @@ struct PercentileState { } void merge(const PercentileState& rhs) { - if (!rhs.inited_flag) { + if (rhs.vec_counts.empty()) { return; } int size_num = cast_set(rhs.vec_quantile.size()); - if (!inited_flag) { + if (vec_counts.empty()) { vec_counts.resize(size_num); vec_quantile = rhs.vec_quantile; inited_flag = true; @@ -736,9 +740,11 @@ struct PercentileExactState { return; } - if (!inited_flag) { + if (values.empty()) { levels = rhs.levels; inited_flag = true; + } else if (rhs.values.empty()) { + return; } else if (UNLIKELY(levels.quantiles != rhs.levels.quantiles)) { throw Exception(ErrorCode::INVALID_ARGUMENT, "percentile aggregate states have incompatible quantiles"); diff --git a/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h b/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h index 4240f24bdb758c..2c2bd0d3e8fc98 100644 --- a/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h +++ b/be/src/exprs/aggregate/aggregate_function_percentile_reservoir.h @@ -45,11 +45,10 @@ struct QuantileReservoirSampler { } void merge(const QuantileReservoirSampler& rhs) { - // NaN samples do not erase the quantile established by a non-null input row. - if (rhs.level == INIT_QUANTILE) { + if (rhs.data.empty()) { return; } - if (level == INIT_QUANTILE) { + if (data.empty()) { level = rhs.level; } else if (UNLIKELY(level != rhs.level)) { throw Exception(ErrorCode::INVALID_ARGUMENT, @@ -59,7 +58,7 @@ struct QuantileReservoirSampler { } void reset() { - level = INIT_QUANTILE; + level = 0.0; data.clear(); } @@ -80,9 +79,7 @@ struct QuantileReservoirSampler { } private: - // Valid quantiles include zero, so use an out-of-range value for fresh/reset states. - static constexpr double INIT_QUANTILE = -1.0; - double level = INIT_QUANTILE; + double level = 0.0; ReservoirSampler data; }; diff --git a/be/src/exprs/aggregate/aggregate_function_sequence_match.h b/be/src/exprs/aggregate/aggregate_function_sequence_match.h index c9838999722427..604d1f107ba3fc 100644 --- a/be/src/exprs/aggregate/aggregate_function_sequence_match.h +++ b/be/src/exprs/aggregate/aggregate_function_sequence_match.h @@ -118,20 +118,17 @@ struct AggregateFunctionSequenceMatchData final { } void merge(const AggregateFunctionSequenceMatchData& other) { - // All-false event rows still establish a pattern that must match during merge. - if (!other.init_flag) { + if (other.events_list.empty()) { return; } - if (!init_flag) { + if (events_list.empty()) { + reset(); init(other.pattern, other.arg_count); } else if (UNLIKELY(pattern != other.pattern || arg_count != other.arg_count)) { throw Exception(ErrorCode::INVALID_ARGUMENT, "sequence aggregate states have incompatible patterns or event counts"); } - if (other.events_list.empty()) { - return; - } events_list.insert(std::end(events_list), std::begin(other.events_list), std::end(other.events_list)); diff --git a/be/src/exprs/aggregate/aggregate_function_topn.h b/be/src/exprs/aggregate/aggregate_function_topn.h index 8cfcde8d422c1f..501b1af198be79 100644 --- a/be/src/exprs/aggregate/aggregate_function_topn.h +++ b/be/src/exprs/aggregate/aggregate_function_topn.h @@ -82,30 +82,21 @@ struct AggregateFunctionTopNData { } void merge(const AggregateFunctionTopNData& rhs) { - if (!rhs.top_num) { + if (!rhs.top_num || rhs.counter_map.empty()) { return; } - if (!top_num) { - top_num = rhs.top_num; - capacity = rhs.capacity; - } else if (UNLIKELY(top_num != rhs.top_num || capacity != rhs.capacity)) { + if (counter_map.empty()) { + *this = rhs; + return; + } + if (UNLIKELY(top_num != rhs.top_num || capacity != rhs.capacity)) { throw Exception(ErrorCode::INVALID_ARGUMENT, "topn aggregate states have incompatible parameters: " "({}, {}) vs ({}, {}) (N, capacity)", top_num, capacity, rhs.top_num, rhs.capacity); } - // Empty payloads still carry configuration. Check it above, then avoid treating - // an empty zero-capacity map as full and adding UINT64_MAX to real counters. - if (rhs.counter_map.empty()) { - return; - } - if (counter_map.empty()) { - counter_map = rhs.counter_map; - return; - } - bool lhs_full = (counter_map.size() >= capacity); bool rhs_full = (rhs.counter_map.size() >= capacity); diff --git a/be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h b/be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h index a7c23e262411fb..b6021c4a6802d3 100644 --- a/be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h +++ b/be/src/exprs/aggregate/aggregate_function_window_funnel_v2.h @@ -124,7 +124,6 @@ struct WindowFunnelStateV2 { int event_count = 0; int64_t window = WINDOW_UNSET; WindowFunnelMode window_funnel_mode = WindowFunnelMode::INVALID; - bool initialized = false; bool sorted = true; std::vector events_list; @@ -134,7 +133,6 @@ struct WindowFunnelStateV2 { void reset() { window = WINDOW_UNSET; window_funnel_mode = WindowFunnelMode::INVALID; - initialized = false; events_list.clear(); sorted = true; } @@ -142,7 +140,6 @@ struct WindowFunnelStateV2 { void add(const IColumn** arg_columns, ssize_t row_num, int64_t win, WindowFunnelMode mode) { window = win; window_funnel_mode = mode; - initialized = true; auto timestamp = assert_cast::ColumnType&, TypeCheckOnRelease::DISABLE>(*arg_columns[2]) @@ -184,38 +181,27 @@ struct WindowFunnelStateV2 { } void merge(const WindowFunnelStateV2& other) { - if (!other.initialized) { + if (other.events_list.empty()) { + return; + } + if (events_list.empty()) { + *this = other; return; } - // All-false input establishes configuration without retaining any events. - if (!initialized) { - window = other.window; - window_funnel_mode = other.window_funnel_mode; - event_count = other.event_count; - initialized = true; - } else if (UNLIKELY(window != other.window || - window_funnel_mode != other.window_funnel_mode)) { + if (UNLIKELY(window != other.window || window_funnel_mode != other.window_funnel_mode)) { throw Exception(ErrorCode::INVALID_ARGUMENT, "window_funnel aggregate states have incompatible window or mode"); } - if (other.events_list.empty()) { - return; - } - if (events_list.empty()) { - events_list = other.events_list; - sorted = other.sorted; - } else { - const auto prefix_size = events_list.size(); - events_list.insert(std::end(events_list), std::begin(other.events_list), - std::end(other.events_list)); - // Both stable_sort and inplace_merge preserve relative order of equal elements. - // Since same-row events have the same timestamp (and thus compare equal in - // the primary sort key), they remain consecutive after merge — preserving - // the validity of continuation flags. - merge_events_list(events_list, prefix_size, sorted, other.sorted); - sorted = true; - } + const auto prefix_size = events_list.size(); + events_list.insert(std::end(events_list), std::begin(other.events_list), + std::end(other.events_list)); + // Both stable_sort and inplace_merge preserve relative order of equal elements. + // Since same-row events have the same timestamp (and thus compare equal in + // the primary sort key), they remain consecutive after merge — preserving + // the validity of continuation flags. + merge_events_list(events_list, prefix_size, sorted, other.sorted); + sorted = true; } void write(BufferWritable& out) const { @@ -223,10 +209,7 @@ struct WindowFunnelStateV2 { write_var_int(window, out); write_var_int(static_cast>(window_funnel_mode), out); - // Tag configured eventless states in the existing sorted field. Legacy readers - // interpret every nonzero value as sorted, so the layout remains compatible. - const auto sorted_flag = static_cast(sorted); - write_var_int(initialized && events_list.empty() ? 2 : sorted_flag, out); + write_var_int(sorted ? 1 : 0, out); write_var_int(cast_set(events_list.size()), out); for (const auto& evt : events_list) { // Use fixed-size binary write for timestamp (8 bytes) and event_idx (1 byte). @@ -248,13 +231,9 @@ struct WindowFunnelStateV2 { read_var_int(tmp, in); sorted = (tmp != 0); - // Legacy states use 0/1 and retain their configuration even without events. - initialized = tmp == 2 || window != WINDOW_UNSET || - window_funnel_mode != WindowFunnelMode::INVALID; Int64 size = 0; read_var_int(size, in); - initialized |= size != 0; events_list.clear(); events_list.resize(size); for (Int64 i = 0; i < size; ++i) { diff --git a/be/src/util/reservoir_sampler.h b/be/src/util/reservoir_sampler.h index a54a2cf89b0fea..d7370dd89755d3 100644 --- a/be/src/util/reservoir_sampler.h +++ b/be/src/util/reservoir_sampler.h @@ -329,6 +329,8 @@ class ReservoirSampler { rng.seed(123456); } + bool empty() const { return total_values == 0; } + double quantileInterpolated(double level) { if (samples.empty()) { return std::numeric_limits::quiet_NaN(); diff --git a/be/test/exprs/aggregate/agg_state_parameters_test.cpp b/be/test/exprs/aggregate/agg_state_parameters_test.cpp index 5457b2488c54c5..c139fb6059030a 100644 --- a/be/test/exprs/aggregate/agg_state_parameters_test.cpp +++ b/be/test/exprs/aggregate/agg_state_parameters_test.cpp @@ -88,6 +88,12 @@ class StateParameterChecks { }); } + void check_direct_mismatch(const Arguments& first, const Arguments& second) { + auto* destination = create(first); + auto* source = create(second); + expect_incompatible([&] { _function->merge(destination, source, _arena); }); + } + void check_merge_result(const Arguments& initial, const Arguments& incoming, bool serialized) { auto* destination = create(); auto* source = create(); @@ -100,13 +106,7 @@ class StateParameterChecks { add(source, incoming); add(expected, incoming); } - if (serialized) { - auto column = serialize(source); - EXPECT_NO_THROW( - _function->deserialize_and_merge_from_column(destination, *column, _arena)); - } else { - EXPECT_NO_THROW(_function->merge(destination, source, _arena)); - } + EXPECT_NO_THROW(merge(destination, source, serialized)); EXPECT_TRUE(ColumnHelper::column_equal(result(destination), result(expected))); } @@ -129,22 +129,27 @@ class StateParameterChecks { EXPECT_TRUE(ColumnHelper::column_equal(result(destination), result(create(second)))); } - void check_mismatch_and_reset(const Arguments& first, const Arguments& second) { - check_mismatch(first, second); - auto* reset_state = create(first); - _function->reset(reset_state); - auto* configured = create(second); - auto serialized = serialize(reset_state); - EXPECT_NO_THROW( - _function->deserialize_and_merge_from_column(configured, *serialized, _arena)); - EXPECT_TRUE(ColumnHelper::column_equal(result(configured), result(create(second)))); - EXPECT_NO_THROW(_function->merge(reset_state, create(second), _arena)); - EXPECT_TRUE(ColumnHelper::column_equal(result(reset_state), result(create(second)))); + void check_noncontributing(const Arguments& empty, const Arguments& populated, + bool decode_empty, bool reverse, bool serialized) { + auto* empty_state = create(empty); + if (decode_empty) { + auto column = serialize(empty_state); + empty_state = create(); + _function->deserialize_and_merge_from_column(empty_state, *column, _arena); + } + auto* destination = reverse ? create(populated) : empty_state; + auto* source = reverse ? empty_state : create(populated); + EXPECT_NO_THROW(merge(destination, source, serialized)); + auto* expected = create(populated); + // A skipped configuration must not poison subsequent contributing merges. + EXPECT_NO_THROW(_function->merge(destination, create(populated), _arena)); + add(expected, populated); + EXPECT_TRUE(ColumnHelper::column_equal(result(destination), result(expected))); } void check_configured_empty_payload(const Arguments& arguments) { // TopN with zero capacity keeps counters in memory but serializes no counters. - // A decoded state must preserve configuration without changing compatible counters. + // A decoded empty state must not change compatible counters. auto serialized = serialize(create(arguments)); auto* configured_empty = create(); _function->deserialize_and_merge_from_column(configured_empty, *serialized, _arena); @@ -188,6 +193,15 @@ class StateParameterChecks { } private: + void merge(AggregateDataPtr destination, AggregateDataPtr source, bool serialized) { + if (serialized) { + auto column = serialize(source); + _function->deserialize_and_merge_from_column(destination, *column, _arena); + } else { + _function->merge(destination, source, _arena); + } + } + AggregateDataPtr create() { auto* place = reinterpret_cast(_arena.alloc(_function->size_of_data())); _function->create(place); @@ -267,6 +281,30 @@ void check_compatible_states(const std::string& name, const Arguments& first, } } } + +void check_ignored_parameters(const std::string& name, const Arguments& empty, + const Arguments& populated, bool decode_empty = false) { + SCOPED_TRACE(name); + DataTypes types; + for (const auto& arg : empty) { + types.push_back(arg.type); + } + auto function = AggregateFunctionSimpleFactory::instance().get( + name, types, nullptr, false, BeExecVersionManager::get_newest_version()); + ASSERT_NE(function, nullptr); + function->set_version(BeExecVersionManager::get_newest_version()); + StateParameterChecks checks(function); + for (bool reverse : {false, true}) { + for (bool serialized : {false, true}) { + SCOPED_TRACE(reverse); + SCOPED_TRACE(serialized); + checks.check_noncontributing(empty, populated, decode_empty, reverse, serialized); + } + } + if (!decode_empty) { + checks.check_empty_and_reset(empty, populated); + } +} } // namespace TEST(AggregateStateParametersTest, TopN) { @@ -317,10 +355,10 @@ TEST(AggregateStateParametersTest, TopNZeroCapacityParameters) { name, types, nullptr, false, BeExecVersionManager::get_newest_version()); ASSERT_NE(function, nullptr); StateParameterChecks checks(function); - // A zero capacity serializes no retained elements but still establishes N/capacity. - checks.check_mismatch(zero_capacity, positive_capacity); - checks.check_mismatch(positive_capacity, zero_capacity); - checks.check_mismatch_and_reset(zero_capacity, positive_capacity); + // Only the serialized zero-capacity state is empty; in-memory counters contribute. + checks.check_direct_mismatch(zero_capacity, positive_capacity); + checks.check_direct_mismatch(positive_capacity, zero_capacity); + check_ignored_parameters(name, zero_capacity, positive_capacity, true); checks.check_merge_result({}, zero_capacity, false); checks.check_merge_result(zero_capacity, {}, false); checks.check_merge_result(zero_capacity, {}, true); @@ -339,7 +377,7 @@ TEST(AggregateStateParametersTest, Percentiles) { {"percentile_array", "percentile_array_v2", "percentile_approx_array"}) { check_parameters(name, {value, quantiles({0.25})}, {value, quantiles({0.75})}); check_parameters(name, {value, quantiles({0.25})}, {value, quantiles({0.25, 0.75})}); - check_parameters(name, {value, quantiles({})}, {value, quantiles({0.25})}); + check_ignored_parameters(name, {value, quantiles({})}, {value, quantiles({0.25})}); } check_parameters("percentile_approx", {value, argument(0.5), argument(2048)}, @@ -354,26 +392,35 @@ TEST(AggregateStateParametersTest, Percentiles) { argument(4096)}); } -TEST(AggregateStateParametersTest, PercentileConfiguredEmptyParameters) { +TEST(AggregateStateParametersTest, PercentileEmptyParametersAreIgnored) { const auto nan = argument(std::numeric_limits::quiet_NaN()); const auto value = argument(7); for (const auto& name : {"percentile_v2", "percentile_approx", "percentile_reservoir"}) { for (double level : {0.0, 0.25, 1.0}) { const auto quantile = argument(level); for (const auto& sample : {nan, value}) { - check_parameters(name, {nan, quantile}, {sample, argument(0.75)}); + check_ignored_parameters(name, {nan, quantile}, + {sample, argument(0.75)}); } check_compatible_states(name, {nan, quantile}, {value, quantile}); } } for (const auto& name : {"percentile_array_v2", "percentile_approx_array"}) { - check_parameters(name, {nan, quantiles({0.25})}, {value, quantiles({0.75})}); - check_parameters(name, {nan, quantiles({0.25})}, {nan, quantiles({0.75})}); + check_ignored_parameters(name, {nan, quantiles({0.25})}, {value, quantiles({0.75})}); + check_ignored_parameters(name, {nan, quantiles({0.25})}, {nan, quantiles({0.75})}); check_compatible_states(name, {nan, quantiles({0.25})}, {value, quantiles({0.25})}); } - check_parameters("percentile_approx_weighted", - {value, argument(0), argument(0.25)}, - {value, argument(1), argument(0.75)}); + check_ignored_parameters( + "percentile_approx_weighted", + {value, argument(0), argument(0.25)}, + {value, argument(1), argument(0.75)}); + check_ignored_parameters( + "percentile_approx", + {nan, argument(0.25), argument(2048)}, + {value, argument(0.75), argument(4096)}); + check_ignored_parameters("percentile_approx_array", + {nan, quantiles({0.25}), argument(2048)}, + {value, quantiles({0.25, 0.75}), argument(4096)}); check_compatible_states("percentile_approx_weighted", {value, argument(0), argument(0.25)}, {value, argument(1), argument(0.25)}); @@ -396,8 +443,10 @@ TEST(AggregateStateParametersTest, CollectAndConcat) { TEST(AggregateStateParametersTest, CollectZeroAndNegativeLimits) { for (const auto& name : {"collect_list", "collect_set"}) { for (const auto& value : {argument(7), argument("a")}) { - for (int limit : {std::numeric_limits::min(), -2, -1, 0, - std::numeric_limits::max()}) { + check_ignored_parameters(name, {value, argument(0)}, + {value, argument(1)}); + for (int limit : + {std::numeric_limits::min(), -2, -1, std::numeric_limits::max()}) { check_parameters(name, {value, argument(limit)}, {value, argument(1)}); } @@ -405,7 +454,9 @@ TEST(AggregateStateParametersTest, CollectZeroAndNegativeLimits) { {value, argument(-2)}); } } - for (int limit : {-2, -1, 0}) { + check_ignored_parameters("collect_list", {quantiles({0.5}), argument(0)}, + {quantiles({0.5}), argument(1)}); + for (int limit : {-2, -1}) { check_parameters("collect_list", {quantiles({0.5}), argument(limit)}, {quantiles({0.5}), argument(1)}); } @@ -503,8 +554,10 @@ TEST(AggregateStateParametersTest, SequenceEventlessParameters) { for (const auto& name : {"sequence_match", "sequence_count"}) { SCOPED_TRACE(name); Arguments eventless {argument("(?1)"), timestamp, no, no}; - check_parameters(name, eventless, {argument("(?2)"), timestamp, yes, no}); - check_parameters(name, eventless, {argument("(?2)"), timestamp, no, no}); + check_ignored_parameters(name, eventless, + {argument("(?2)"), timestamp, yes, no}); + check_ignored_parameters(name, eventless, + {argument("(?2)"), timestamp, no, no}); DataTypes types; for (const auto& arg : eventless) { types.push_back(arg.type); @@ -536,12 +589,18 @@ TEST(AggregateStateParametersTest, WindowFunnelEventlessParameters) { Arguments eventless {argument(0), argument("default"), timestamp, no, no}; for (const auto& event : {yes, no}) { - check_parameters(name, eventless, - {argument(3), argument("default"), - timestamp, event, no}); - check_parameters(name, eventless, - {argument(0), argument("fixed"), - timestamp, event, no}); + // V1 retains all-false rows: they can break a chain in fixed mode. + auto check = [&](const Arguments& incoming) { + if (std::string(name) == "window_funnel_v1") { + check_parameters(name, eventless, incoming); + } else { + check_ignored_parameters(name, eventless, incoming); + } + }; + check({argument(3), argument("default"), timestamp, + event, no}); + check({argument(0), argument("fixed"), timestamp, event, + no}); } DataTypes types; for (const auto& arg : eventless) { @@ -562,8 +621,7 @@ TEST(AggregateStateParametersTest, WindowFunnelEventlessParameters) { } } } - // Even arguments that equal the fresh-state sentinels establish configuration. - check_parameters( + check_ignored_parameters( "window_funnel_v2", {argument(-1), argument("invalid"), timestamp, no, no}, {argument(0), argument("default"), timestamp, no, no}); diff --git a/be/test/exprs/aggregate/vec_window_funnel_v2_test.cpp b/be/test/exprs/aggregate/vec_window_funnel_v2_test.cpp index 9cf8917a45490f..6bcc3b99dbd5ea 100644 --- a/be/test/exprs/aggregate/vec_window_funnel_v2_test.cpp +++ b/be/test/exprs/aggregate/vec_window_funnel_v2_test.cpp @@ -133,7 +133,6 @@ void check_legacy_eventless_configuration(int64_t window, WindowFunnelMode mode) VectorBufferReader reader(buffer.get_data_at(0)); WindowFunnelStateV2 state; state.read(reader); - EXPECT_EQ(state.initialized, window != -1 || mode != WindowFunnelMode::INVALID); EXPECT_EQ(state.window, window); EXPECT_EQ(state.window_funnel_mode, mode); EXPECT_EQ(state.event_count, 2); @@ -141,9 +140,22 @@ void check_legacy_eventless_configuration(int64_t window, WindowFunnelMode mode) WindowFunnelStateV2 destination; destination.merge(state); - EXPECT_EQ(destination.initialized, state.initialized); - EXPECT_EQ(destination.window, window); - EXPECT_EQ(destination.window_funnel_mode, mode); + EXPECT_EQ(destination.window, WindowFunnelStateV2::WINDOW_UNSET); + EXPECT_EQ(destination.window_funnel_mode, WindowFunnelMode::INVALID); + + DateV2Value time; + time.unchecked_set_time(2024, 1, 1, 0, 0, 0, 0); + WindowFunnelStateV2 populated(2); + populated.window = 10; + populated.window_funnel_mode = WindowFunnelMode::DEFAULT; + populated.events_list.push_back({time.to_date_int_val(), 1}); + EXPECT_NO_THROW(state.merge(populated)); + EXPECT_EQ(state.window, populated.window); + EXPECT_EQ(state.window_funnel_mode, populated.window_funnel_mode); + EXPECT_EQ(state.get(), 1); + state.reset(); + EXPECT_NO_THROW(populated.merge(state)); + EXPECT_EQ(populated.get(), 1); } void check_configured_eventless_encoding(bool reset) { @@ -156,7 +168,7 @@ void check_configured_eventless_encoding(bool reset) { const IColumn* columns[] = {nullptr, nullptr, timestamp.get(), event.get()}; WindowFunnelStateV2 state(1); - // These values collide with fresh-state sentinels, so the serialized tag is necessary. + // Empty payloads use the original boolean sorted field without an initialization tag. state.add(columns, 0, -1, WindowFunnelMode::INVALID); if (reset) { state.reset(); @@ -176,7 +188,7 @@ void check_configured_eventless_encoding(bool reset) { read_var_int(value, legacy_reader); EXPECT_EQ(value, static_cast(WindowFunnelMode::INVALID)); read_var_int(value, legacy_reader); - EXPECT_TRUE(value != 0); // Legacy readers decode the sorted flag this way. + EXPECT_EQ(value, 1); read_var_int(value, legacy_reader); EXPECT_EQ(value, 0); read_var_int(value, legacy_reader); @@ -185,7 +197,6 @@ void check_configured_eventless_encoding(bool reset) { VectorBufferReader reader(buffer.get_data_at(0)); WindowFunnelStateV2 restored; restored.read(reader); - EXPECT_EQ(restored.initialized, !reset); EXPECT_TRUE(restored.events_list.empty()); EXPECT_TRUE(restored.sorted); read_var_int(value, reader); @@ -193,7 +204,7 @@ void check_configured_eventless_encoding(bool reset) { } } // namespace -TEST(VWindowFunnelV2SerializationTest, LegacyEventlessConfiguration) { +TEST(VWindowFunnelV2SerializationTest, IgnoreEventlessConfiguration) { for (int64_t window : {-1, 0, 3}) { for (auto mode : {WindowFunnelMode::INVALID, WindowFunnelMode::DEFAULT}) { check_legacy_eventless_configuration(window, mode); @@ -201,7 +212,7 @@ TEST(VWindowFunnelV2SerializationTest, LegacyEventlessConfiguration) { } } -TEST(VWindowFunnelV2SerializationTest, ConfiguredEventlessEncodingKeepsLegacyLayout) { +TEST(VWindowFunnelV2SerializationTest, EventlessEncodingUsesBooleanSortedFlag) { check_configured_eventless_encoding(false); check_configured_eventless_encoding(true); } diff --git a/docs/aggregate-state-parameters.md b/docs/aggregate-state-parameters.md index 9d78ccd9184130..bc03c24e41363d 100644 --- a/docs/aggregate-state-parameters.md +++ b/docs/aggregate-state-parameters.md @@ -1,37 +1,37 @@ # Aggregate-state parameter compatibility -Parameterized aggregate states distinguish initialization from retained data: - -- A fresh state has no configuration. It is an identity during merge. -- A non-null input establishes configuration even when it retains no effective - data. Merging two initialized states requires compatible parameter values. -- `reset()` discards both data and configuration. The resulting state is an - identity, including after serialization and deserialization. - -These rules apply in both merge orders and to `_merge` and `_union`. Examples of -initialized states include a percentile reservoir with only NaN samples, an exact -percentile V2 with only NaN samples, a zero-weight approximate percentile, a sequence -or funnel with all-false events, a zero-limit collection, an empty-string -`group_concat`, an empty bitmap intersection, and a zero-half-decay moving average. -TopN retains its N/capacity configuration even when a zero capacity causes its -serialized payload to contain no elements. After configuration checks, empty -TopN payloads do not change compatible counters; this also prevents zero-capacity -empty maps from applying an invalid full-map count adjustment. - -Negative collection limits retain the existing unlimited-collection behavior; -they are configurations and must match when states are merged. A zero EMA -half-decay still produces zero, but it is a configuration rather than an identity. +Merging two aggregate states that contribute data requires compatible parameters. +States whose payload can be skipped do not participate in parameter checks. When +the destination has no contributing data, it adopts the contributing source's +parameters. These rules apply in both merge orders, including `_merge` and `_union`. + +Non-contributing states include: + +- TopN states with no retained counters, including serialized zero-capacity states. +- Limited `collect_list` and `collect_set` states with no retained elements. +- Percentile reservoirs, exact percentile V2 states and approximate percentiles + with no retained samples, including all-NaN samples and zero-weight inputs. +- Percentile arrays with no quantile levels and therefore no retained samples. +- Sequence functions and Window Funnel V2 states with no matched events. + +Empty percentile arrays may still retain their output shape when all merged +states have no samples. Their parameters never constrain a contributing state. +An empty TopN map is handled before full-map count adjustment, avoiding invalid +counter changes when capacity is zero. + +An empty final result does not always imply a skippable state: + +- `group_concat('')` retains an input string and can contribute a separator. +- `intersect_count` retains filter keys even when their bitmaps are empty; those + keys affect the intersection. +- Window Funnel V1 stores all-false rows, which can interrupt a fixed-mode chain. +- EMA with zero half-decay retains its accumulated value and reference time. + +These contributing states still require matching parameters. Negative collection +limits retain unlimited-collection behavior and must match for nonempty states. Invalid parameters remain subject to each function's existing validation. -Existing initialization flags are reused where available. Reservoir reserves -quantile `-1` for fresh/reset states, outside the valid interval `[0, 1]`. Limited -collection states reserve one below the minimum Int32 value, outside their -parameter domain and within the serialized varint range. EMA tracks initialization -independently in memory and reserves a serialized NaN half-decay for fresh/reset -states; configured NaN half-decays cannot be serialized. -The existing serialized field layouts are retained. - -Historical states from the trial AggState implementation are outside this change's -compatibility scope. In particular, old reset states may contain stale configuration -values that cannot always be distinguished from initialized states. The rules above -are guaranteed for states produced by the updated implementation. +Fresh states and states cleared by `reset()` are merge identities. Serialized +field layouts are retained. Window Funnel V2 uses the original boolean sorted +field; no initialization tag is needed for eventless states. Historical states +from the trial AggState implementation remain outside the compatibility scope. diff --git a/regression-test/data/datatype_p0/agg_state/test_agg_state_parameters.out b/regression-test/data/datatype_p0/agg_state/test_agg_state_parameters.out index 5e03dc7d603597..369bed39a2036d 100644 --- a/regression-test/data/datatype_p0/agg_state/test_agg_state_parameters.out +++ b/regression-test/data/datatype_p0/agg_state/test_agg_state_parameters.out @@ -1,4 +1,400 @@ -- This file is automatically generated. You should know what you did if you want to edit this +-- !empty_0_percentile_approx_weighted_merge -- +7 + +-- !empty_0_percentile_approx_weighted_union -- +7 + +-- !empty_1_percentile_approx_weighted_merge -- +7 + +-- !empty_1_percentile_approx_weighted_union -- +7 + +-- !empty_2_percentile_array_merge -- +[7] + +-- !empty_2_percentile_array_union -- +[7] + +-- !empty_3_percentile_array_merge -- +[7] + +-- !empty_3_percentile_array_union -- +[7] + +-- !empty_4_topn_weighted_merge -- +[1] + +-- !empty_4_topn_weighted_union -- +[1] + +-- !empty_5_topn_weighted_merge -- +[1] + +-- !empty_5_topn_weighted_union -- +[1] + +-- !empty_6_topn_array_merge -- +[1] + +-- !empty_6_topn_array_union -- +[1] + +-- !empty_7_topn_array_merge -- +[1] + +-- !empty_7_topn_array_union -- +[1] + +-- !empty_8_topn_merge -- +{"a":1} + +-- !empty_8_topn_union -- +{"a":1} + +-- !empty_9_topn_merge -- +{"a":1} + +-- !empty_9_topn_union -- +{"a":1} + +-- !empty_10_percentile_reservoir_merge -- +NaN + +-- !empty_10_percentile_reservoir_union -- +NaN + +-- !empty_11_percentile_reservoir_merge -- +NaN + +-- !empty_11_percentile_reservoir_union -- +NaN + +-- !empty_12_percentile_reservoir_merge -- +7 + +-- !empty_12_percentile_reservoir_union -- +7 + +-- !empty_13_percentile_reservoir_merge -- +7 + +-- !empty_13_percentile_reservoir_union -- +7 + +-- !empty_14_percentile_reservoir_merge -- +NaN + +-- !empty_14_percentile_reservoir_union -- +NaN + +-- !empty_15_percentile_reservoir_merge -- +NaN + +-- !empty_15_percentile_reservoir_union -- +NaN + +-- !empty_16_percentile_reservoir_merge -- +7 + +-- !empty_16_percentile_reservoir_union -- +7 + +-- !empty_17_percentile_reservoir_merge -- +7 + +-- !empty_17_percentile_reservoir_union -- +7 + +-- !empty_18_percentile_reservoir_merge -- +NaN + +-- !empty_18_percentile_reservoir_union -- +NaN + +-- !empty_19_percentile_reservoir_merge -- +NaN + +-- !empty_19_percentile_reservoir_union -- +NaN + +-- !empty_20_percentile_reservoir_merge -- +7 + +-- !empty_20_percentile_reservoir_union -- +7 + +-- !empty_21_percentile_reservoir_merge -- +7 + +-- !empty_21_percentile_reservoir_union -- +7 + +-- !empty_22_collect_list_merge -- +[7] + +-- !empty_22_collect_list_union -- +[7] + +-- !empty_23_collect_list_merge -- +[7] + +-- !empty_23_collect_list_union -- +[7] + +-- !empty_24_collect_list_merge -- +["a"] + +-- !empty_24_collect_list_union -- +["a"] + +-- !empty_25_collect_list_merge -- +["a"] + +-- !empty_25_collect_list_union -- +["a"] + +-- !empty_26_collect_set_merge -- +[7] + +-- !empty_26_collect_set_union -- +[7] + +-- !empty_27_collect_set_merge -- +[7] + +-- !empty_27_collect_set_union -- +[7] + +-- !empty_28_collect_set_merge -- +["a"] + +-- !empty_28_collect_set_union -- +["a"] + +-- !empty_29_collect_set_merge -- +["a"] + +-- !empty_29_collect_set_union -- +["a"] + +-- !empty_30_collect_list_merge -- +[[7]] + +-- !empty_30_collect_list_union -- +[[7]] + +-- !empty_31_collect_list_merge -- +[[7]] + +-- !empty_31_collect_list_union -- +[[7]] + +-- !empty_32_sequence_match_merge -- +false + +-- !empty_32_sequence_match_union -- +false + +-- !empty_33_sequence_match_merge -- +false + +-- !empty_33_sequence_match_union -- +false + +-- !empty_34_sequence_match_merge -- +false + +-- !empty_34_sequence_match_union -- +false + +-- !empty_35_sequence_match_merge -- +false + +-- !empty_35_sequence_match_union -- +false + +-- !empty_36_sequence_count_merge -- +0 + +-- !empty_36_sequence_count_union -- +0 + +-- !empty_37_sequence_count_merge -- +0 + +-- !empty_37_sequence_count_union -- +0 + +-- !empty_38_sequence_count_merge -- +0 + +-- !empty_38_sequence_count_union -- +0 + +-- !empty_39_sequence_count_merge -- +0 + +-- !empty_39_sequence_count_union -- +0 + +-- !empty_40_window_funnel_merge -- +1 + +-- !empty_40_window_funnel_union -- +1 + +-- !empty_41_window_funnel_merge -- +1 + +-- !empty_41_window_funnel_union -- +1 + +-- !empty_42_window_funnel_merge -- +1 + +-- !empty_42_window_funnel_union -- +1 + +-- !empty_43_window_funnel_merge -- +1 + +-- !empty_43_window_funnel_union -- +1 + +-- !empty_44_window_funnel_merge -- +0 + +-- !empty_44_window_funnel_union -- +0 + +-- !empty_45_window_funnel_merge -- +0 + +-- !empty_45_window_funnel_union -- +0 + +-- !empty_46_window_funnel_merge -- +0 + +-- !empty_46_window_funnel_union -- +0 + +-- !empty_47_window_funnel_merge -- +0 + +-- !empty_47_window_funnel_union -- +0 + +-- !empty_48_window_funnel_v2_merge -- +1 + +-- !empty_48_window_funnel_v2_union -- +1 + +-- !empty_49_window_funnel_v2_merge -- +1 + +-- !empty_49_window_funnel_v2_union -- +1 + +-- !empty_50_window_funnel_v2_merge -- +1 + +-- !empty_50_window_funnel_v2_union -- +1 + +-- !empty_51_window_funnel_v2_merge -- +1 + +-- !empty_51_window_funnel_v2_union -- +1 + +-- !empty_52_window_funnel_v2_merge -- +0 + +-- !empty_52_window_funnel_v2_union -- +0 + +-- !empty_53_window_funnel_v2_merge -- +0 + +-- !empty_53_window_funnel_v2_union -- +0 + +-- !empty_54_window_funnel_v2_merge -- +0 + +-- !empty_54_window_funnel_v2_union -- +0 + +-- !empty_55_window_funnel_v2_merge -- +0 + +-- !empty_55_window_funnel_v2_union -- +0 + +-- !empty_56_percentile_approx_merge -- +7 + +-- !empty_56_percentile_approx_union -- +7 + +-- !empty_57_percentile_approx_merge -- +7 + +-- !empty_57_percentile_approx_union -- +7 + +-- !empty_58_percentile_approx_array_merge -- +[7, 7] + +-- !empty_58_percentile_approx_array_union -- +[7, 7] + +-- !empty_59_percentile_approx_array_merge -- +[7, 7] + +-- !empty_59_percentile_approx_array_union -- +[7, 7] + +-- !empty_60_percentile_approx_array_merge -- +[7] + +-- !empty_60_percentile_approx_array_union -- +[7] + +-- !empty_61_percentile_approx_array_merge -- +[7] + +-- !empty_61_percentile_approx_array_union -- +[7] + +-- !empty_62_percentile_approx_merge -- +7 + +-- !empty_62_percentile_approx_union -- +7 + +-- !empty_63_percentile_approx_merge -- +7 + +-- !empty_63_percentile_approx_union -- +7 + +-- !empty_64_percentile_approx_array_merge -- +[7] + +-- !empty_64_percentile_approx_array_union -- +[7] + +-- !empty_65_percentile_approx_array_merge -- +[7] + +-- !empty_65_percentile_approx_array_union -- +[7] + -- !reservoir_compatible_nan -- 100 diff --git a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy index 92b2108783dfac..88dbde8ee63e2d 100644 --- a/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy +++ b/regression-test/suites/datatype_p0/agg_state/test_agg_state_parameters.groovy @@ -59,29 +59,31 @@ suite("test_agg_state_parameters") { } // Each pair has the same AggState type but incompatible configuration values. + def ignoredCases = [ + ["percentile_approx_weighted", "7, 0, 0.25", "7, 1, 0.75"], + ["percentile_array", "7, cast([] as array)", "7, [0.25]"], + ["topn_weighted", "1, 1, 1, 0", "1, 1, 1, 2"], + ["topn_array", "1, 1, 0", "1, 1, 2"], + ["topn", "'a', 1, 0", "'a', 1, 2"] + ] def cases = [ ["topn", "'a', 1", "'a', 3"], ["topn", "'a', 3, 2", "'a', 3, 5"], ["topn", "'a', 1, 6", "'a', 3, 2"], - ["topn", "'a', 1, 0", "'a', 1, 2"], ["topn_array", "1, 1", "1, 3"], ["topn_array", "1, 3, 2", "1, 3, 5"], ["topn_array", "'a', 1, 6", "'a', 3, 2"], - ["topn_array", "1, 1, 0", "1, 1, 2"], ["topn_weighted", "1, 1, 1", "1, 1, 3"], ["topn_weighted", "1, 1, 3, 2", "1, 1, 3, 5"], - ["topn_weighted", "1, 1, 1, 0", "1, 1, 1, 2"], ["histogram", "7, 1", "7, 3"], ["percentile", "7, 0.25", "7, 0.75"], ["percentile_array", "7, [0.25]", "7, [0.75]"], ["percentile_array", "7, [0.25]", "7, [0.25, 0.75]"], - ["percentile_array", "7, cast([] as array)", "7, [0.25]"], ["percentile_approx", "7, 0.25", "7, 0.75"], ["percentile_approx", "7, 0.5, 2048", "7, 0.5, 4096"], ["percentile_approx_array", "7, [0.25]", "7, [0.75]"], ["percentile_approx_array", "7, [0.25], 2048", "7, [0.25], 4096"], ["percentile_approx_weighted", "7, 1, 0.25", "7, 1, 0.75"], - ["percentile_approx_weighted", "7, 0, 0.25", "7, 1, 0.75"], ["percentile_approx_weighted", "7, 1, 0.5, 2048", "7, 1, 0.5, 4096"], ["percentile_reservoir", "7, 0.25", "7, 0.75"], ["collect_list", "7, 1", "7, 3"], @@ -103,29 +105,29 @@ suite("test_agg_state_parameters") { ["sequence_count", "'(?1)', non_nullable(cast('2024-01-01' as datetime)), true, false", "'(?2)', non_nullable(cast('2024-01-01' as datetime)), true, false"] ] - // A non-null input establishes configuration even if no sample is retained. + // States without contributing samples ignore their parameter values. // Keep AggState argument nullability identical across both sides of the UNION. for (def quantile : ["0.0", "0.25", "1.0"]) { for (def sample : ["'NaN'", "7"]) { - cases.add(["percentile_reservoir", "non_nullable(cast('NaN' as double)), ${quantile}", + ignoredCases.add(["percentile_reservoir", "non_nullable(cast('NaN' as double)), ${quantile}", "non_nullable(cast(${sample} as double)), 0.75"]) } } for (def function : ["collect_list", "collect_set"]) { for (def value : ["7", "'a'"]) { for (def limit : [-2, -1, 0]) { - cases.add([function, "${value}, ${limit}", "${value}, 1"]) + (limit == 0 ? ignoredCases : cases).add([function, "${value}, ${limit}", "${value}, 1"]) } cases.add([function, "${value}, -1", "${value}, -2"]) } } for (def limit : [-2, -1, 0]) { - cases.add(["collect_list", "[7], ${limit}", "[7], 1"]) + (limit == 0 ? ignoredCases : cases).add(["collect_list", "[7], ${limit}", "[7], 1"]) } - // All-false event rows retain their pattern, including when both states have no events. + // Sequence and V2 funnel states discard all-false event rows. for (def function : ["sequence_match", "sequence_count"]) { for (def event : ["true", "false"]) { - cases.add([function, + ignoredCases.add([function, "'(?1)', non_nullable(cast('2024-01-01' as datetime)), false, false", "'(?2)', non_nullable(cast('2024-01-01' as datetime)), ${event}, false"]) } @@ -136,11 +138,12 @@ suite("test_agg_state_parameters") { "3, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), true, false"]) cases.add([function, "1, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), true, false", "1, cast('fixed' as string), non_nullable(cast('2024-01-01' as datetime)), true, false"]) - // V2 stores no events for an all-false row, but its configuration still participates. + // V1 keeps these rows because they can interrupt a fixed-mode chain. + def eventlessCases = function == "window_funnel_v1" ? cases : ignoredCases for (def event : ["true", "false"]) { - cases.add([function, "0, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), false, false", + eventlessCases.add([function, "0, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), false, false", "3, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), ${event}, false"]) - cases.add([function, "0, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), false, false", + eventlessCases.add([function, "0, cast('default' as string), non_nullable(cast('2024-01-01' as datetime)), false, false", "0, cast('fixed' as string), non_nullable(cast('2024-01-01' as datetime)), ${event}, false"]) } } @@ -164,6 +167,35 @@ suite("test_agg_state_parameters") { } } + ignoredCases.add(["percentile_approx", "non_nullable(cast('NaN' as double)), 0.25", + "cast(7 as double), 0.75"]) + ignoredCases.add(["percentile_approx_array", "non_nullable(cast('NaN' as double)), [0.25]", + "cast(7 as double), [0.25, 0.75]"]) + ignoredCases.add(["percentile_approx_array", "cast(7 as double), cast([] as array)", + "cast(7 as double), [0.75]"]) + ignoredCases.add(["percentile_approx", "non_nullable(cast('NaN' as double)), 0.25, 2048", + "cast(7 as double), 0.75, 4096"]) + ignoredCases.add(["percentile_approx_array", "non_nullable(cast('NaN' as double)), [0.25], 2048", + "cast(7 as double), [0.75], 4096"]) + int emptyCase = 0 + for (def entry : ignoredCases) { + def function = entry[0] + for (def args : [[entry[1], entry[2]], [entry[2], entry[1]]]) { + for (def suffix : ["merge", "union"]) { + def merged = """ + SELECT ${function}_${suffix}(s) AS s FROM ( + SELECT ${function}_state(${args[0]}) AS s + UNION ALL + SELECT ${function}_state(${args[1]}) AS s + ) states + """ + def query = suffix == "union" ? "SELECT ${function}_merge(s) FROM (${merged}) merged" : merged + "order_qt_empty_${emptyCase}_${function}_${suffix}"(query) + } + emptyCase++ + } + } + order_qt_reservoir_compatible_nan """ SELECT percentile_reservoir_merge(s) FROM ( SELECT percentile_reservoir_state(non_nullable(cast('NaN' as double)), 0.25) s From 7e1a126ca022f001a57292fb4e6866917c71fa9a Mon Sep 17 00:00:00 2001 From: happenlee Date: Sat, 12 Sep 2026 14:03:38 +0800 Subject: [PATCH 14/16] [fix](fe) Preserve aggregate state layouts across rewrites ### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: Constant folding can replace a nullable string cast with a non-null literal. AggState producers then recomputed their input layout while UNION consumers retained the analyzed nullable layout, returning NULL instead of aggregate values or incompatible-parameter errors. Preserve the analyzed state signature for _state and _combine and align combine raw inputs with it. Explicit state casts still rebind layouts, and expression identity includes the declared state type. Cover these paths together with the rule that empty reservoir samples do not constrain merge parameters. ### Release note AggState expressions preserve nullable layouts across constant folding, preventing incorrect NULL results and skipped parameter mismatch errors. ### Check List (For Author) - Test: FE build and Checkstyle passed; 43 FE unit tests and all 13 AggState regression suites passed. Regression expectations generated and verified with the repository runner; git diff whitespace check passed. - Behavior changed: Yes, folded AggState expressions return correct values or parameter errors, while non-contributing empty states ignore parameters. - Does this need documentation: No --- .../glue/translator/ExpressionTranslator.java | 32 +++- .../expression/rules/ConvertAggStateCast.java | 6 +- .../combinator/CombineCombinator.java | 16 +- .../functions/combinator/StateCombinator.java | 16 +- .../combinator/StateCombinatorTest.java | 157 ++++++++++++++++ .../test_agg_state_nullable_rewrite.out | 97 ++++++++++ .../test_agg_state_nullable_rewrite.groovy | 169 ++++++++++++++++++ 7 files changed, 479 insertions(+), 14 deletions(-) create mode 100644 fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinatorTest.java create mode 100644 regression-test/data/datatype_p0/agg_state/test_agg_state_nullable_rewrite.out create mode 100644 regression-test/suites/datatype_p0/agg_state/test_agg_state_nullable_rewrite.groovy diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/ExpressionTranslator.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/ExpressionTranslator.java index 95d305f8d68547..162bece0228660 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/ExpressionTranslator.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/ExpressionTranslator.java @@ -101,6 +101,7 @@ import org.apache.doris.nereids.trees.expressions.functions.scalar.DictGetMany; import org.apache.doris.nereids.trees.expressions.functions.scalar.ElementAt; import org.apache.doris.nereids.trees.expressions.functions.scalar.Lambda; +import org.apache.doris.nereids.trees.expressions.functions.scalar.Nullable; import org.apache.doris.nereids.trees.expressions.functions.scalar.ScalarFunction; import org.apache.doris.nereids.trees.expressions.functions.udf.JavaUdaf; import org.apache.doris.nereids.trees.expressions.functions.udf.JavaUdf; @@ -111,6 +112,7 @@ import org.apache.doris.nereids.trees.expressions.functions.window.WindowFunction; import org.apache.doris.nereids.trees.expressions.literal.Literal; import org.apache.doris.nereids.trees.expressions.visitor.DefaultExpressionVisitor; +import org.apache.doris.nereids.types.AggStateType; import org.apache.doris.nereids.types.DataType; import org.apache.doris.thrift.TDictFunction; @@ -838,20 +840,15 @@ public Expr visitStateCombinator(StateCombinator combinator, PlanTranslatorConte visitAggregateFunction(combinator.getNestedFunction(), context).getFn(), new FunctionParams(false, arguments), isReturnNullable); return convertToStateCombinator(combinator.getName(), functionCallExpr, - arguments.stream().map(Expr::getType).collect(Collectors.toList()), - combinator.getArguments().stream().map(Expression::nullable).collect(Collectors.toList()), - isReturnNullable); + combinator.getDataType().toCatalogDataType()); } - private FunctionCallExpr convertToStateCombinator(String name, FunctionCallExpr fnCall, - List argTypes, List argNullables, - boolean returnNullable) { + private FunctionCallExpr convertToStateCombinator(String name, FunctionCallExpr fnCall, Type returnType) { Function aggFunction = fnCall.getFn(); List arguments = Arrays.asList(aggFunction.getArgs()); org.apache.doris.catalog.ScalarFunction fn = new org.apache.doris.catalog.ScalarFunction( new FunctionName(name), arguments, - Expr.createAggStateType(aggFunction.getFunctionName().getFunction(), - argTypes, argNullables, returnNullable), + returnType, aggFunction.hasVarArgs(), aggFunction.isUserVisible()); fn.setNullableMode(NullableMode.ALWAYS_NOT_NULLABLE); fn.setBinaryType(Function.BinaryType.AGG_STATE); @@ -1015,6 +1012,25 @@ public Expr visitPythonUdtf(PythonUdtf udtf, PlanTranslatorContext context) { private Expr translateAggregateFunction(AggregateFunction function, List currentPhaseArguments, List aggFnArguments, AggregateParam aggregateParam, PlanTranslatorContext context) { + if (function instanceof CombineCombinator) { + AggStateType stateType = (AggStateType) function.getDataType(); + List nullables = stateType.getSubTypeNullables(); + // The state layout survives rewrites even when a cast folds to a non-null literal. + // _combine consumes columns directly, so align both its signature and raw inputs here. + for (int i = 0; i < aggFnArguments.size(); i++) { + aggFnArguments.set(i, new SlotRef( + stateType.getSubTypes().get(i).toCatalogDataType(), nullables.get(i))); + } + if (!aggregateParam.aggMode.consumeAggregateBuffer) { + ImmutableList.Builder arguments = + ImmutableList.builderWithExpectedSize(currentPhaseArguments.size()); + for (int i = 0; i < currentPhaseArguments.size(); i++) { + Expression argument = currentPhaseArguments.get(i); + arguments.add(nullables.get(i) && !argument.nullable() ? new Nullable(argument) : argument); + } + currentPhaseArguments = arguments.build(); + } + } List currentPhaseCatalogArguments = Lists.newArrayListWithCapacity(currentPhaseArguments.size()); for (Expression arg : currentPhaseArguments) { if (arg instanceof OrderExpression) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/ConvertAggStateCast.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/ConvertAggStateCast.java index bdb0b1ce93f10c..fda66e42bd1252 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/ConvertAggStateCast.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/ConvertAggStateCast.java @@ -28,6 +28,7 @@ import org.apache.doris.nereids.trees.expressions.functions.scalar.Nullable; import org.apache.doris.nereids.types.AggStateType; import org.apache.doris.nereids.types.DataType; +import org.apache.doris.nereids.util.MoreFieldsThread; import org.apache.doris.nereids.util.TypeCoercionUtils; import com.google.common.collect.ImmutableList; @@ -74,7 +75,10 @@ public static Expression convert(Cast cast) { } newChildren.add(newChild); } - child = child.withChildren(newChildren.build()); + // An explicit state cast changes the serialized layout, unlike ordinary rewrites. + StateCombinator state = (StateCombinator) child; + child = MoreFieldsThread.keepFunctionSignature(false, + () -> state.withChildren(newChildren.build())); return cast.withChildren(ImmutableList.of(child)); } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/CombineCombinator.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/CombineCombinator.java index 039f1ef8e07061..bac88910e340fd 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/CombineCombinator.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/CombineCombinator.java @@ -96,7 +96,7 @@ private static AggStateType createReturnType(List arguments, Aggrega @Override public CombineCombinator withChildren(List children) { - return new CombineCombinator(getFunctionParams(children), nested); + return new CombineCombinator(getFunctionParams(children), nested.withChildren(children)); } @Override @@ -104,7 +104,7 @@ public AggregateFunction withDistinctAndChildren(boolean distinct, List R accept(ExpressionVisitor visitor, C context) { @Override public DataType getDataType() { - return returnType; + return getSignature().returnType; + } + + @Override + public boolean equals(Object other) { + return super.equals(other) && getDataType().equals(((CombineCombinator) other).getDataType()); + } + + @Override + public int computeHashCode() { + return Objects.hash(super.computeHashCode(), getDataType()); } @Override diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinator.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinator.java index 04c671101fa7df..0f266a4f370c22 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinator.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinator.java @@ -102,7 +102,7 @@ public static StateCombinator create(AggregateFunction nested) { @Override public StateCombinator withChildren(List children) { - return new StateCombinator(getFunctionParams(children), nested); + return new StateCombinator(getFunctionParams(children), nested.withChildren(children)); } @Override @@ -119,7 +119,19 @@ public R accept(ExpressionVisitor visitor, C context) { @Override public DataType getDataType() { - return returnType; + // Input nullability is part of the serialized state layout. Keep the analyzed + // signature when rewrites replace nullable expressions with non-null literals. + return getSignature().returnType; + } + + @Override + protected boolean extraEquals(Expression that) { + return super.extraEquals(that) && getDataType().equals(that.getDataType()); + } + + @Override + public int computeHashCode() { + return Objects.hash(super.computeHashCode(), getDataType()); } @Override diff --git a/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinatorTest.java b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinatorTest.java new file mode 100644 index 00000000000000..067e46f1eb960c --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/nereids/trees/expressions/functions/combinator/StateCombinatorTest.java @@ -0,0 +1,157 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.nereids.trees.expressions.functions.combinator; + +import org.apache.doris.analysis.FunctionCallExpr; +import org.apache.doris.nereids.glue.translator.ExpressionTranslator; +import org.apache.doris.nereids.glue.translator.PlanTranslatorContext; +import org.apache.doris.nereids.rules.expression.rules.ConvertAggStateCast; +import org.apache.doris.nereids.rules.expression.rules.FoldConstantRuleOnFE; +import org.apache.doris.nereids.trees.expressions.AggregateExpression; +import org.apache.doris.nereids.trees.expressions.Cast; +import org.apache.doris.nereids.trees.expressions.Expression; +import org.apache.doris.nereids.trees.expressions.functions.agg.AggregateParam; +import org.apache.doris.nereids.trees.expressions.functions.agg.PercentileReservoir; +import org.apache.doris.nereids.trees.expressions.literal.DoubleLiteral; +import org.apache.doris.nereids.trees.expressions.literal.VarcharLiteral; +import org.apache.doris.nereids.types.AggStateType; +import org.apache.doris.nereids.types.DoubleType; +import org.apache.doris.nereids.util.MemoTestUtils; +import org.apache.doris.nereids.util.MoreFieldsThread; +import org.apache.doris.nereids.util.PlanChecker; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +class StateCombinatorTest { + @BeforeEach + void setUp() { + MemoTestUtils.createConnectContext(); + } + + @Test + void testConstantFoldingPreservesStateLayout() { + StateCombinator state = StateCombinator.create(new PercentileReservoir( + new Cast(new VarcharLiteral("7"), DoubleType.INSTANCE), new DoubleLiteral(0.25))); + AggStateType analyzedType = (AggStateType) state.getDataType(); + Assertions.assertEquals(ImmutableList.of(true, false), analyzedType.getSubTypeNullables()); + + StateCombinator folded = (StateCombinator) MoreFieldsThread.keepFunctionSignature( + () -> FoldConstantRuleOnFE.evaluateWithoutContext(state)); + Assertions.assertEquals(new DoubleLiteral(7), folded.child(0)); + Assertions.assertFalse(folded.child(0).nullable()); + Assertions.assertEquals(analyzedType, folded.getDataType()); + + FunctionCallExpr translated = (FunctionCallExpr) ExpressionTranslator.translate( + folded, new PlanTranslatorContext()); + Assertions.assertEquals(analyzedType.toCatalogDataType(), translated.getFn().getReturnType()); + } + + @Test + void testCombineConstantFoldingPreservesInputLayout() { + PercentileReservoir nested = new PercentileReservoir( + new Cast(new VarcharLiteral("7"), DoubleType.INSTANCE), new DoubleLiteral(0.25)); + CombineCombinator combine = new CombineCombinator(nested.children(), nested); + AggStateType analyzedType = (AggStateType) combine.getDataType(); + CombineCombinator folded = (CombineCombinator) MoreFieldsThread.keepFunctionSignature( + () -> FoldConstantRuleOnFE.evaluateWithoutContext(combine)); + Assertions.assertFalse(folded.child(0).nullable()); + Assertions.assertEquals(new DoubleLiteral(7), folded.child(0)); + Assertions.assertEquals(analyzedType, folded.getDataType()); + Assertions.assertEquals(folded, MoreFieldsThread.keepFunctionSignature( + () -> folded.withDistinctAndChildren(false, folded.children()))); + FunctionCallExpr translated = (FunctionCallExpr) ExpressionTranslator.translate( + new AggregateExpression(folded, AggregateParam.LOCAL_RESULT), new PlanTranslatorContext()); + Assertions.assertEquals(analyzedType.toCatalogDataType(), translated.getFn().getReturnType()); + Assertions.assertTrue(translated.getChild(0).isNullable()); + } + + @Test + void testAnalysisCanRecomputeStateType() { + StateCombinator state = StateCombinator.create(new PercentileReservoir( + new Cast(new VarcharLiteral("7"), DoubleType.INSTANCE), new DoubleLiteral(0.25))); + state.getSignature(); + StateCombinator rebound = MoreFieldsThread.keepFunctionSignature(false, + () -> state.withChildren(ImmutableList.of(new DoubleLiteral(7), new DoubleLiteral(0.25)))); + Assertions.assertEquals(ImmutableList.of(false, false), + ((AggStateType) rebound.getDataType()).getSubTypeNullables()); + } + + @Test + void testStateLayoutsHaveDistinctExpressionIdentity() { + PercentileReservoir nullable = new PercentileReservoir( + new Cast(new VarcharLiteral("7"), DoubleType.INSTANCE), new DoubleLiteral(0.25)); + PercentileReservoir nonnullable = new PercentileReservoir(new DoubleLiteral(7), new DoubleLiteral(0.25)); + for (boolean combine : ImmutableList.of(false, true)) { + Expression beforeFold = combine ? new CombineCombinator(nullable.children(), nullable) + : StateCombinator.create(nullable); + Expression direct = combine ? new CombineCombinator(nonnullable.children(), nonnullable) + : StateCombinator.create(nonnullable); + Expression folded = MoreFieldsThread.keepFunctionSignature( + () -> FoldConstantRuleOnFE.evaluateWithoutContext(beforeFold)); + Assertions.assertEquals(direct.children(), folded.children()); + Assertions.assertNotEquals(direct.getDataType(), folded.getDataType()); + Assertions.assertNotEquals(direct, folded); + Assertions.assertEquals(2, ImmutableSet.of(direct, folded).size()); + Assertions.assertEquals(folded, MoreFieldsThread.keepFunctionSignature( + () -> folded.withChildren(folded.children()))); + } + } + + @Test + void testExplicitStateCastAfterConstantFolding() { + PlanChecker.from(MemoTestUtils.createConnectContext()) + .analyze("select cast(percentile_reservoir_state(cast('7' as double), 0.25) " + + "as agg_state)") + .rewrite(); + } + + @Test + void testExplicitStateCastRebindsCustomSignature() { + for (String nullable : ImmutableList.of("null", "not null")) { + PlanChecker.from(MemoTestUtils.createConnectContext()) + .analyze("select cast(max_state(null) as agg_state)") + .rewrite(); + } + } + + @Test + void testExplicitStateCastUpdatesStateLayout() { + for (boolean nullable : ImmutableList.of(false, true)) { + Expression argument = nullable ? new DoubleLiteral(7) + : new Cast(new VarcharLiteral("7"), DoubleType.INSTANCE); + StateCombinator state = StateCombinator.create( + new PercentileReservoir(argument, new DoubleLiteral(0.25))); + AggStateType target = new AggStateType("percentile_reservoir", + ImmutableList.of(DoubleType.INSTANCE, DoubleType.INSTANCE), + ImmutableList.of(nullable, false), true); + Assertions.assertNotEquals(target, state.getDataType()); + + Cast converted = (Cast) MoreFieldsThread.keepFunctionSignature( + () -> ConvertAggStateCast.convert(new Cast(state, target))); + Assertions.assertEquals(nullable, converted.child().child(0).nullable()); + Assertions.assertEquals(target, converted.child().getDataType()); + FunctionCallExpr translated = (FunctionCallExpr) ExpressionTranslator.translate( + converted.child(), new PlanTranslatorContext()); + Assertions.assertEquals(target.toCatalogDataType(), translated.getFn().getReturnType()); + } + } +} diff --git a/regression-test/data/datatype_p0/agg_state/test_agg_state_nullable_rewrite.out b/regression-test/data/datatype_p0/agg_state/test_agg_state_nullable_rewrite.out new file mode 100644 index 00000000000000..587378bed203ec --- /dev/null +++ b/regression-test/data/datatype_p0/agg_state/test_agg_state_nullable_rewrite.out @@ -0,0 +1,97 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !state_compatible -- +7.25 + +-- !state_union -- +7.25 + +-- !state_empty_NaN_true -- +NaN + +-- !state_empty_union_NaN_true -- +NaN + +-- !state_empty_NaN_false -- +NaN + +-- !state_empty_union_NaN_false -- +NaN + +-- !state_empty_7_true -- +7 + +-- !state_empty_union_7_true -- +7 + +-- !state_empty_7_false -- +7 + +-- !state_empty_union_7_false -- +7 + +-- !combine_compatible -- +7.25 + +-- !combine_union -- +7.25 + +-- !combine_empty_NaN_true -- +NaN + +-- !combine_empty_union_NaN_true -- +NaN + +-- !combine_empty_NaN_false -- +NaN + +-- !combine_empty_union_NaN_false -- +NaN + +-- !combine_empty_7_true -- +7 + +-- !combine_empty_union_7_true -- +7 + +-- !combine_empty_7_false -- +7 + +-- !combine_empty_union_7_false -- +7 + +-- !state_mixed_true -- +7.25 + +-- !state_mixed_false -- +7.25 + +-- !combine_mixed_true -- +7.25 + +-- !combine_mixed_false -- +7.25 + +-- !combine_phase_1 -- +7 + +-- !combine_phase_2 -- +7 + +-- !combine_parameter_cast -- +{"a":2} + +-- !state_different_layouts -- +7.25 7.25 + +-- !combine_different_layouts -- +7.25 7.25 + +-- !null_input -- +7 + +-- !sum -- +15 + +-- !stored_cast -- +7.25 7.25 15 + diff --git a/regression-test/suites/datatype_p0/agg_state/test_agg_state_nullable_rewrite.groovy b/regression-test/suites/datatype_p0/agg_state/test_agg_state_nullable_rewrite.groovy new file mode 100644 index 00000000000000..54845b4aa2fe27 --- /dev/null +++ b/regression-test/suites/datatype_p0/agg_state/test_agg_state_nullable_rewrite.groovy @@ -0,0 +1,169 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("test_agg_state_nullable_rewrite") { + sql "set enable_agg_state=true" + + // Folding the string casts must preserve the nullable inputs in the state layout. + for (def producer : ["state", "combine"]) { + "order_qt_${producer}_compatible"(""" + SELECT percentile_reservoir_merge(s) + FROM ( + SELECT percentile_reservoir_${producer}(cast('7' AS double), 0.25) s + UNION ALL + SELECT percentile_reservoir_${producer}(cast('8' AS double), 0.25) s + ) states + """) + "order_qt_${producer}_union"(""" + SELECT percentile_reservoir_merge(s) + FROM ( + SELECT percentile_reservoir_union(s) s + FROM ( + SELECT percentile_reservoir_${producer}(cast('7' AS double), 0.25) s + UNION ALL + SELECT percentile_reservoir_${producer}(cast('8' AS double), 0.25) s + ) states + ) united + """) + for (def levels : [["0.25", "0.75"], ["0.75", "0.25"]]) { + for (def consumer : ["merge", "union"]) { + test { + sql """ + SELECT percentile_reservoir_${consumer}(s) + FROM ( + SELECT percentile_reservoir_${producer}(cast('7' AS double), ${levels[0]}) s + UNION ALL + SELECT percentile_reservoir_${producer}(cast('8' AS double), ${levels[1]}) s + ) states + """ + exception "incompatible" + } + } + } + // NaN contributes no sample, so an empty state cannot constrain the quantile. + for (def value : ["NaN", "7"]) { + for (def emptyFirst : [true, false]) { + def empty = "SELECT percentile_reservoir_${producer}(cast('NaN' AS double), 0.0) s" + def other = "SELECT percentile_reservoir_${producer}(cast('${value}' AS double), 0.75) s" + def branches = emptyFirst ? "${empty} UNION ALL ${other}" : "${other} UNION ALL ${empty}" + "order_qt_${producer}_empty_${value}_${emptyFirst}"(""" + SELECT percentile_reservoir_merge(s) FROM (${branches}) states + """) + "order_qt_${producer}_empty_union_${value}_${emptyFirst}"(""" + SELECT percentile_reservoir_merge(s) + FROM (SELECT percentile_reservoir_union(s) s FROM (${branches}) states) united + """) + } + } + } + + sql "DROP TABLE IF EXISTS test_agg_state_nullable_input" + sql """ + CREATE TABLE test_agg_state_nullable_input (id INT, v DOUBLE NULL) + DUPLICATE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ("replication_num" = "1") + """ + sql "INSERT INTO test_agg_state_nullable_input VALUES (1, 8), (2, NULL)" + for (def producer : ["state", "combine"]) { + for (def constantFirst : [true, false]) { + def constant = "SELECT percentile_reservoir_${producer}(cast('7' AS double), 0.25) s" + def table = """ + SELECT percentile_reservoir_${producer}(v, 0.25) s + FROM test_agg_state_nullable_input + """ + def branches = constantFirst ? "${constant} UNION ALL ${table}" : "${table} UNION ALL ${constant}" + "order_qt_${producer}_mixed_${constantFirst}"(""" + SELECT percentile_reservoir_merge(s) FROM (${branches}) states + """) + } + } + + for (def phase : [1, 2]) { + "order_qt_combine_phase_${phase}"(""" + SELECT /*+ SET_VAR(agg_phase=${phase}) */ percentile_reservoir_merge(s) + FROM ( + SELECT id, percentile_reservoir_combine(cast('7' AS double), 0.25) s + FROM test_agg_state_nullable_input GROUP BY id + ) states + """) + } + order_qt_combine_parameter_cast """ + SELECT topn_merge(s) FROM ( + SELECT topn_combine('a', cast('1' AS int)) s + FROM test_agg_state_nullable_input + ) states + """ + + // The folded arguments are equal, but the two state layouts must not be deduplicated. + for (def producer : ["state", "combine"]) { + "order_qt_${producer}_different_layouts"(""" + SELECT percentile_reservoir_merge(nullable_state), percentile_reservoir_merge(nonnull_state) + FROM ( + SELECT percentile_reservoir_${producer}(cast('7' AS double), 0.25) nullable_state, + percentile_reservoir_${producer}(cast(7 AS double), 0.25) nonnull_state + UNION ALL + SELECT percentile_reservoir_${producer}(cast('8' AS double), 0.25) nullable_state, + percentile_reservoir_${producer}(cast(8 AS double), 0.25) nonnull_state + ) states + """) + } + + order_qt_null_input """ + SELECT percentile_reservoir_merge(s) + FROM ( + SELECT percentile_reservoir_state(cast(NULL AS double), 0.25) s + UNION ALL + SELECT percentile_reservoir_state(cast('7' AS double), 0.25) s + ) states + """ + order_qt_sum """ + SELECT sum_merge(s) FROM ( + SELECT sum_state(cast('7' AS int)) s + UNION ALL + SELECT sum_state(cast('8' AS int)) s + ) states + """ + + // Explicit casts must still retarget the producer, including when inserting into a state column. + sql "DROP TABLE IF EXISTS test_agg_state_nullable_cast" + sql """ + CREATE TABLE test_agg_state_nullable_cast ( + id INT, + nullable_state AGG_STATE GENERIC, + nonnull_state AGG_STATE GENERIC, + widened_state AGG_STATE GENERIC + ) AGGREGATE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ("replication_num" = "1") + """ + sql """ + INSERT INTO test_agg_state_nullable_cast + SELECT 1, percentile_reservoir_state(cast(7 AS double), 0.25), + percentile_reservoir_state(cast('7' AS double), 0.25), sum_state(cast(7 AS int)) + """ + sql """ + INSERT INTO test_agg_state_nullable_cast + SELECT 2, percentile_reservoir_state(cast(8 AS double), 0.25), + percentile_reservoir_state(cast('8' AS double), 0.25), sum_state(cast(8 AS int)) + """ + order_qt_stored_cast """ + SELECT percentile_reservoir_merge(nullable_state), percentile_reservoir_merge(nonnull_state), + sum_merge(widened_state) + FROM test_agg_state_nullable_cast + """ +} From c3da1036090188708720960b4f98afb9cffd1bef Mon Sep 17 00:00:00 2001 From: happenlee Date: Sun, 13 Sep 2026 00:30:53 +0800 Subject: [PATCH 15/16] [fix](be) Restore compact collect state initialization ### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: Collect states serialize max_size even when no limit is specified. Moving the fresh-state marker outside the Int32 input range grew this field from two bytes to six, including its length byte. This added four bytes to every newly serialized no-limit collect state, including aliases and array_agg with non-nullable inputs. Restore the original -1 defaults and initialization checks for all collect specializations. Keep reset restoring -1, ignore empty sources, let empty destinations adopt the source limit, and reject different limits before merging two populated states. This also rejects distinct negative limits while preserving unlimited collection for matching negative limits. Cover exact serialization sizes for fresh, populated, deserialized and reused states across numeric, string and complex list paths, set paths and aliases. Also cover incompatible negative limits for complex lists. ### Release note Restore compact serialization for collect_list, collect_set and array_agg states with non-nullable inputs when no limit is specified. ### Check List (For Author) - Test: - Unit Test: 11 BE ASAN tests passed across VAggCollectTest, AggregateFunctionCollectTest and the collect cases in AggregateStateParametersTest. - Regression test: test_agg_state_parameters and array_agg passed with zero failures, fatal errors or skips; existing output files were unchanged. - Manual test: BE ASAN build, clang-format 16.0.6, header hygiene and git diff --check passed. clang-tidy reported the existing core/types.h unmatched NOLINTEND error for both files; no changed-line diagnostic was reported, but the complete check did not pass. - Behavior changed: Yes, restore the compact serialized limit marker; SQL parameter compatibility rules are unchanged. - Does this need documentation: No, internal representation change preserving the documented merge rules. --- .../aggregate/aggregate_function_collect.h | 57 ++++++++----------- .../aggregate/agg_state_parameters_test.cpp | 45 +++++++++++++++ 2 files changed, 70 insertions(+), 32 deletions(-) diff --git a/be/src/exprs/aggregate/aggregate_function_collect.h b/be/src/exprs/aggregate/aggregate_function_collect.h index 8eedf4b555fa02..ad46b8d50c06a1 100644 --- a/be/src/exprs/aggregate/aggregate_function_collect.h +++ b/be/src/exprs/aggregate/aggregate_function_collect.h @@ -46,24 +46,15 @@ #include "util/var_int.h" namespace doris { -struct AggregateFunctionCollectLimitData { - // Limits are Int32 inputs. Keep all of them, including negative limits, distinct from - // the fresh/reset marker while retaining the existing Int64 serialized field. - static constexpr Int64 UNINITIALIZED_MAX_SIZE = - static_cast(std::numeric_limits::min()) - 1; - Int64 max_size = UNINITIALIZED_MAX_SIZE; - - bool is_initialized() const { return max_size != UNINITIALIZED_MAX_SIZE; } -}; - template -struct AggregateFunctionCollectSetData : AggregateFunctionCollectLimitData { +struct AggregateFunctionCollectSetData { static constexpr PrimitiveType PType = T; using ElementType = typename PrimitiveTypeTraits::CppType; using ColVecType = typename PrimitiveTypeTraits::ColumnType; using SelfType = AggregateFunctionCollectSetData; using Set = doris::flat_hash_set; Set data_set; + Int64 max_size = -1; AggregateFunctionCollectSetData(const DataTypes& argument_types) {} @@ -76,7 +67,7 @@ struct AggregateFunctionCollectSetData : AggregateFunctionCollectLimitData { void merge(const SelfType& rhs) { if constexpr (HasLimit) { - if (!is_initialized()) { + if (max_size == -1) { max_size = rhs.max_size; } @@ -122,19 +113,20 @@ struct AggregateFunctionCollectSetData : AggregateFunctionCollectLimitData { void reset() { data_set.clear(); - max_size = UNINITIALIZED_MAX_SIZE; + max_size = -1; } }; template requires(is_string_type(T)) -struct AggregateFunctionCollectSetData : AggregateFunctionCollectLimitData { +struct AggregateFunctionCollectSetData { static constexpr PrimitiveType PType = T; using ElementType = StringRef; using ColVecType = ColumnString; using SelfType = AggregateFunctionCollectSetData; using Set = doris::flat_hash_set; Set data_set; + Int64 max_size = -1; AggregateFunctionCollectSetData(const DataTypes& argument_types) {} @@ -147,7 +139,7 @@ struct AggregateFunctionCollectSetData : AggregateFunctionCollectLi } void merge(const SelfType& rhs, Arena& arena) { - if (!is_initialized()) { + if (max_size == -1) { max_size = rhs.max_size; } @@ -192,17 +184,18 @@ struct AggregateFunctionCollectSetData : AggregateFunctionCollectLi void reset() { data_set.clear(); - max_size = UNINITIALIZED_MAX_SIZE; + max_size = -1; } }; template -struct AggregateFunctionCollectListData : AggregateFunctionCollectLimitData { +struct AggregateFunctionCollectListData { static constexpr PrimitiveType PType = T; using ElementType = typename PrimitiveTypeTraits::CppType; using ColVecType = typename PrimitiveTypeTraits::ColumnType; using SelfType = AggregateFunctionCollectListData; PaddedPODArray data; + Int64 max_size = -1; AggregateFunctionCollectListData(const DataTypes& argument_types) {} @@ -216,7 +209,7 @@ struct AggregateFunctionCollectListData : AggregateFunctionCollectLimitData { void merge(const SelfType& rhs) { if constexpr (HasLimit) { - if (!is_initialized()) { + if (max_size == -1) { max_size = rhs.max_size; } for (auto& rhs_elem : rhs.data) { @@ -246,7 +239,7 @@ struct AggregateFunctionCollectListData : AggregateFunctionCollectLimitData { void reset() { data.clear(); - max_size = UNINITIALIZED_MAX_SIZE; + max_size = -1; } void insert_result_into(IColumn& to) const { @@ -259,11 +252,12 @@ struct AggregateFunctionCollectListData : AggregateFunctionCollectLimitData { template requires(is_string_type(T)) -struct AggregateFunctionCollectListData : AggregateFunctionCollectLimitData { +struct AggregateFunctionCollectListData { static constexpr PrimitiveType PType = T; using ElementType = StringRef; using ColVecType = ColumnString; MutableColumnPtr data; + Int64 max_size = -1; AggregateFunctionCollectListData(const DataTypes& argument_types) { data = ColVecType::create(); @@ -275,7 +269,7 @@ struct AggregateFunctionCollectListData : AggregateFunctionCollectL void merge(const AggregateFunctionCollectListData& rhs) { if constexpr (HasLimit) { - if (!is_initialized()) { + if (max_size == -1) { max_size = rhs.max_size; } @@ -314,7 +308,7 @@ struct AggregateFunctionCollectListData : AggregateFunctionCollectL void reset() { data->clear(); - max_size = UNINITIALIZED_MAX_SIZE; + max_size = -1; } void insert_result_into(IColumn& to) const { @@ -326,12 +320,13 @@ struct AggregateFunctionCollectListData : AggregateFunctionCollectL template requires(!is_string_type(T) && !is_int_or_bool(T) && !is_float_or_double(T) && !is_decimal(T) && !is_date_type(T) && !is_timestamp_ns_type(T) && !is_ip(T) && !is_timestamptz_type(T)) -struct AggregateFunctionCollectListData : AggregateFunctionCollectLimitData { +struct AggregateFunctionCollectListData { static constexpr PrimitiveType PType = T; using ElementType = StringRef; using Self = AggregateFunctionCollectListData; DataTypeSerDeSPtr serde; // for complex serialize && deserialize from multi BE MutableColumnPtr column_data; + Int64 max_size = -1; AggregateFunctionCollectListData(const DataTypes& argument_types) { DataTypePtr column_type = argument_types[0]; @@ -345,7 +340,7 @@ struct AggregateFunctionCollectListData : AggregateFunctionCollectL void merge(const AggregateFunctionCollectListData& rhs) { if constexpr (HasLimit) { - if (!is_initialized()) { + if (max_size == -1) { max_size = rhs.max_size; } @@ -407,7 +402,7 @@ struct AggregateFunctionCollectListData : AggregateFunctionCollectL void reset() { column_data->clear(); - max_size = UNINITIALIZED_MAX_SIZE; + max_size = -1; } void insert_result_into(IColumn& to) const { to.insert_range_from(*column_data, 0, size()); } @@ -444,7 +439,7 @@ class AggregateFunctionCollect final Arena& arena) const override { auto& data = this->data(place); if constexpr (HasLimit) { - if (!data.is_initialized()) { + if (data.max_size == -1) { data.max_size = assert_cast(columns[1]) ->get_element(row_num); @@ -470,12 +465,10 @@ class AggregateFunctionCollect final } if (data.size() == 0) { data.max_size = rhs_data.max_size; - } else { - if (UNLIKELY(data.max_size != rhs_data.max_size)) { - throw Exception(ErrorCode::INVALID_ARGUMENT, - "{} aggregate states have incompatible limits: {} vs {}", - get_name(), data.max_size, rhs_data.max_size); - } + } else if (UNLIKELY(data.max_size != rhs_data.max_size)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "{} aggregate states have incompatible limits: {} vs {}", + get_name(), data.max_size, rhs_data.max_size); } } if constexpr (ENABLE_ARENA) { diff --git a/be/test/exprs/aggregate/agg_state_parameters_test.cpp b/be/test/exprs/aggregate/agg_state_parameters_test.cpp index c139fb6059030a..f3c2bbe5019190 100644 --- a/be/test/exprs/aggregate/agg_state_parameters_test.cpp +++ b/be/test/exprs/aggregate/agg_state_parameters_test.cpp @@ -174,6 +174,24 @@ class StateParameterChecks { EXPECT_TRUE(ColumnHelper::column_equal(result(destination), result(expected))); } + void check_serialization_size(const Arguments& arguments, size_t empty_size, + size_t populated_size) { + auto* state = create(); + for (int reuse = 0; reuse < 2; ++reuse) { + SCOPED_TRACE(reuse); + EXPECT_EQ(serialize(state)->get_data_at(0).size, empty_size); + add(state, arguments); + auto serialized = serialize(state); + EXPECT_EQ(serialized->get_data_at(0).size, populated_size); + + auto* restored = create(); + _function->deserialize_and_merge_from_column(restored, *serialized, _arena); + EXPECT_TRUE(ColumnHelper::column_equal(result(restored), result(state))); + EXPECT_EQ(serialize(restored)->get_data_at(0).size, populated_size); + _function->reset(state); + } + } + void check_invalid_outputs(const Arguments& arguments, const std::string& message) { auto* state = create(arguments); for (bool serialize_state : {false, true}) { @@ -460,6 +478,33 @@ TEST(AggregateStateParametersTest, CollectZeroAndNegativeLimits) { check_parameters("collect_list", {quantiles({0.5}), argument(limit)}, {quantiles({0.5}), argument(1)}); } + check_parameters("collect_list", {quantiles({0.5}), argument(-1)}, + {quantiles({0.5}), argument(-2)}); +} + +TEST(AggregateStateParametersTest, CollectNoLimitSerializationSize) { + auto check = [](const char* name, const ColumnWithTypeAndName& value, size_t empty_size, + size_t populated_size) { + SCOPED_TRACE(name); + SCOPED_TRACE(value.type->get_name()); + auto function = AggregateFunctionSimpleFactory::instance().get( + name, {value.type}, nullptr, false, BeExecVersionManager::get_newest_version()); + ASSERT_NE(function, nullptr); + function->set_version(BeExecVersionManager::get_newest_version()); + StateParameterChecks checks(function); + checks.check_serialization_size({value}, empty_size, populated_size); + }; + + // The legacy -1 field takes two bytes: one length byte and one ZigZag byte. + for (const auto* name : {"collect_list", "group_array", "array_agg"}) { + check(name, argument(7), 4, 8); + check(name, argument("a"), 6, 7 + sizeof(IColumn::Offset)); + check(name, quantiles({}), sizeof(size_t) + 2, sizeof(size_t) + 6); + } + for (const auto* name : {"collect_set", "group_uniq_array"}) { + check(name, argument(7), 4, 8); + check(name, argument("a"), 4, 7); + } } TEST(AggregateStateParametersTest, GroupConcatEmptyStrings) { From bf483fc348a1ea28c511e24fc1719f4162e71ad1 Mon Sep 17 00:00:00 2001 From: happenlee Date: Sun, 13 Sep 2026 11:25:17 +0800 Subject: [PATCH 16/16] [fix](be) Return empty arrays for sample-free approximate percentiles ### What problem does this PR solve? Issue Number: N/A Related PR: #67805 Problem Summary: percentile_approx_array retained quantile metadata after all NaN samples were discarded. Merging two sample-free states with different quantile counts adopted the right-hand metadata, so distributed merge order changed the final NaN-array length and serialized union state. Return an empty array whenever no samples are retained and serialize those states using the existing fresh-state encoding. Skip sample-free sources and let sample-free destinations adopt contributing sources. Compare quantiles and compression only when both states contribute samples. Cover intermediate empty results, direct and serialized merge orders, association, later contributors, reset, ordinary aggregation and state/combine/merge/union paths. ### Release note percentile_approx_array now returns [] when no samples are retained, including all-NaN input, instead of retaining a NaN for each requested quantile. Empty states ignore parameter differences; contributing states still require matching quantiles and compression. ### Check List (For Author) - Test: - Unit Test: 9 BE ASAN tests passed across AggregateFunctionPercentileApproxArrayTest and the percentile tests in AggregateStateParametersTest. - Regression test: All 14 datatype_p0/agg_state suites and test_aggregate_percentile_approx_array passed with zero failures, fatal errors or skips. The new suite contains 45 generated result sets and 16 expected errors; generated output passed a normal rerun. - Manual test: BE ASAN build, clang-format 16.0.6, check-format, header hygiene and whitespace checks passed (allowing the regression runner's trailing blank separator). clang-tidy remains blocked by the existing core/types.h unmatched NOLINTEND error; none of the emitted diagnostics is on a changed line. - Behavior changed: Yes, sample-free percentile_approx_array results are always empty arrays and their serialized states use the fresh-state encoding. - Does this need documentation: Yes, updated docs/aggregate-state-parameters.md. --- .../aggregate/aggregate_function_percentile.h | 42 ++--- .../exprs/aggregate/agg_percentile_test.cpp | 6 +- .../aggregate/agg_state_parameters_test.cpp | 74 +++++++++ docs/aggregate-state-parameters.md | 9 +- .../test_percentile_approx_array_empty.out | 144 ++++++++++++++++++ .../test_percentile_approx_array_empty.groovy | 117 ++++++++++++++ 6 files changed, 361 insertions(+), 31 deletions(-) create mode 100644 regression-test/data/datatype_p0/agg_state/test_percentile_approx_array_empty.out create mode 100644 regression-test/suites/datatype_p0/agg_state/test_percentile_approx_array_empty.groovy diff --git a/be/src/exprs/aggregate/aggregate_function_percentile.h b/be/src/exprs/aggregate/aggregate_function_percentile.h index 5bc34c39e35c9e..4983ecbd808ff3 100644 --- a/be/src/exprs/aggregate/aggregate_function_percentile.h +++ b/be/src/exprs/aggregate/aggregate_function_percentile.h @@ -332,6 +332,8 @@ class AggregateFunctionPercentileApproxWeightedFourParams final }; struct PercentileApproxArrayState { + bool has_samples() const { return !levels.empty() && digest->total_size() != 0; } + void init(const PaddedPODArray& quantiles, const NullMap& null_map, size_t start, size_t size, float compression = 10000) { if (init_flag) { @@ -369,16 +371,15 @@ struct PercentileApproxArrayState { } void write(BufferWritable& buf) const { - buf.write_binary(init_flag); - if (!init_flag) { + // Sample-free states share the fresh-state encoding, independent of their parameters. + const bool has_data = has_samples(); + buf.write_binary(has_data); + if (!has_data) { return; } levels.write(buf); buf.write_binary(compressions); - if (levels.empty()) { - return; - } uint32_t serialize_size = digest->serialized_size(); std::string result(serialize_size, '0'); digest->serialize(reinterpret_cast(result.data())); @@ -404,33 +405,22 @@ struct PercentileApproxArrayState { } void merge(const PercentileApproxArrayState& rhs) { - if (!rhs.init_flag) { + if (!rhs.has_samples()) { return; } - // Preserve the result shape when every state is empty, but let contributing - // samples replace parameters recorded by an empty destination. - if (!init_flag || levels.empty() || digest->total_size() == 0) { + if (!has_samples()) { levels = rhs.levels; compressions = rhs.compressions; - if (!levels.empty()) { - digest = TDigest::create_unique(compressions); - } + digest = TDigest::create_unique(compressions); init_flag = true; - } else if (rhs.levels.empty() || rhs.digest->total_size() == 0) { - return; - } else { - if (UNLIKELY(compressions != rhs.compressions || - levels.quantiles != rhs.levels.quantiles)) { - throw Exception( - ErrorCode::INVALID_ARGUMENT, - "percentile_approx_array aggregate states have incompatible quantiles " - "or compression"); - } - } - if (!levels.empty()) { - digest->merge(rhs.digest.get()); + } else if (UNLIKELY(compressions != rhs.compressions || + levels.quantiles != rhs.levels.quantiles)) { + throw Exception(ErrorCode::INVALID_ARGUMENT, + "percentile_approx_array aggregate states have incompatible quantiles " + "or compression"); } + digest->merge(rhs.digest.get()); } void reset() { @@ -442,7 +432,7 @@ struct PercentileApproxArrayState { void insert_result_into(IColumn& to) const { auto& column_data = assert_cast(to).get_data(); - if (levels.empty()) { + if (!has_samples()) { return; } diff --git a/be/test/exprs/aggregate/agg_percentile_test.cpp b/be/test/exprs/aggregate/agg_percentile_test.cpp index 8e42fc2cc5e51e..bfb911bdb4c9db 100644 --- a/be/test/exprs/aggregate/agg_percentile_test.cpp +++ b/be/test/exprs/aggregate/agg_percentile_test.cpp @@ -111,7 +111,7 @@ void expect_results_equal(const std::vector& actual, const std::vector values {1, 2, 3, 4, 5, 100}; + const std::vector values {1, 2, 3, 4, 5, 100, std::numeric_limits::quiet_NaN()}; const std::vector quantiles {0.9, 0.0, 0.5, 0.5, 1.0}; auto function = create_percentile_approx_array_function(false); ASSERT_NE(function, nullptr); @@ -255,7 +255,7 @@ TEST(AggregateFunctionPercentileApproxArrayTest, EmptyQuantilesAndInvalidQuantil function->deserialize(restored_place, reader, arena); const auto& restored_state = *reinterpret_cast(restored_place); - EXPECT_TRUE(restored_state.init_flag); + EXPECT_FALSE(restored_state.init_flag); EXPECT_EQ(restored_state.digest.get(), nullptr); EXPECT_TRUE(read_result(function, restored_place).empty()); @@ -264,7 +264,7 @@ TEST(AggregateFunctionPercentileApproxArrayTest, EmptyQuantilesAndInvalidQuantil function->create(merged_place); function->merge(merged_place, restored_place, arena); const auto& merged_state = *reinterpret_cast(merged_place); - EXPECT_TRUE(merged_state.init_flag); + EXPECT_FALSE(merged_state.init_flag); EXPECT_EQ(merged_state.digest.get(), nullptr); EXPECT_TRUE(read_result(function, merged_place).empty()); diff --git a/be/test/exprs/aggregate/agg_state_parameters_test.cpp b/be/test/exprs/aggregate/agg_state_parameters_test.cpp index f3c2bbe5019190..b1c500e846fce2 100644 --- a/be/test/exprs/aggregate/agg_state_parameters_test.cpp +++ b/be/test/exprs/aggregate/agg_state_parameters_test.cpp @@ -192,6 +192,37 @@ class StateParameterChecks { } } + void check_sample_free_arrays(const Arguments& first, const Arguments& second, + const Arguments& populated) { + for (bool serialized : {false, true}) { + SCOPED_TRACE(serialized); + auto* left = create(first); + auto* right = create(second); + check_empty_array_state(left); + check_empty_array_state(right); + merge(left, right, serialized); + check_empty_array_state(left); + + // Check the empty intermediate before a later contributor can hide its shape. + auto* restored = create(); + auto empty_column = serialize(left); + _function->deserialize_and_merge_from_column(restored, *empty_column, _arena); + check_empty_array_state(restored); + merge(restored, create(populated), serialized); + EXPECT_TRUE(ColumnHelper::column_equal(result(restored), result(create(populated)))); + + // Both association orders must adopt the contributor's parameters. + merge(left, create(populated), serialized); + merge(right, create(populated), serialized); + auto* other_left = create(first); + merge(other_left, right, serialized); + EXPECT_TRUE(ColumnHelper::column_equal(result(left), result(other_left))); + EXPECT_TRUE(ColumnHelper::column_equal(result(left), result(create(populated)))); + _function->reset(left); + check_empty_array_state(left); + } + } + void check_invalid_outputs(const Arguments& arguments, const std::string& message) { auto* state = create(arguments); for (bool serialize_state : {false, true}) { @@ -211,6 +242,15 @@ class StateParameterChecks { } private: + void check_empty_array_state(AggregateDataPtr place) { + const auto output = result(place); + const auto& array = assert_cast(*output); + EXPECT_EQ(array.size(), 1); + EXPECT_EQ(array.get_data().size(), 0); + EXPECT_EQ(serialize(place)->get_data_at(0).to_string(), + serialize(create())->get_data_at(0).to_string()); + } + void merge(AggregateDataPtr destination, AggregateDataPtr source, bool serialized) { if (serialized) { auto column = serialize(source); @@ -444,6 +484,40 @@ TEST(AggregateStateParametersTest, PercentileEmptyParametersAreIgnored) { {value, argument(1), argument(0.25)}); } +TEST(AggregateStateParametersTest, PercentileApproxArraySampleFreeStates) { + const auto nan = argument(std::numeric_limits::quiet_NaN()); + const auto value = argument(7); + for (bool has_compression : {false, true}) { + SCOPED_TRACE(has_compression); + std::vector empty_cases { + {nan, quantiles({0.25})}, {nan, quantiles({0.25, 0.75})}, {value, quantiles({})}}; + Arguments populated {value, quantiles({0.1, 0.5, 0.9})}; + if (has_compression) { + for (size_t i = 0; i < empty_cases.size(); ++i) { + empty_cases[i].push_back(argument(2048 * (i + 1))); + } + populated.push_back(argument(10000)); + } + DataTypes types; + for (const auto& arg : populated) { + types.push_back(arg.type); + } + auto function = AggregateFunctionSimpleFactory::instance().get( + "percentile_approx_array", types, nullptr, false, + BeExecVersionManager::get_newest_version()); + ASSERT_NE(function, nullptr); + StateParameterChecks checks(function); + for (const auto& first : empty_cases) { + for (const auto& second : empty_cases) { + checks.check_sample_free_arrays(first, second, populated); + } + } + } + check_parameters("percentile_approx_array", + {value, quantiles({0.25}), argument(2048)}, + {value, quantiles({0.25}), argument(4096)}); +} + TEST(AggregateStateParametersTest, CollectAndConcat) { for (const auto& name : {"collect_list", "collect_set"}) { for (const auto& value : {argument(7), argument("a")}) { diff --git a/docs/aggregate-state-parameters.md b/docs/aggregate-state-parameters.md index bc03c24e41363d..23c6fd0ff3f6be 100644 --- a/docs/aggregate-state-parameters.md +++ b/docs/aggregate-state-parameters.md @@ -14,8 +14,13 @@ Non-contributing states include: - Percentile arrays with no quantile levels and therefore no retained samples. - Sequence functions and Window Funnel V2 states with no matched events. -Empty percentile arrays may still retain their output shape when all merged -states have no samples. Their parameters never constrain a contributing state. +`percentile_approx_array` returns `[]` when it has no retained samples, including +all-NaN input and empty quantile arrays. Sample-free states use the fresh-state +serialized encoding and do not constrain a contributing state's parameters. +Only two contributing states must have matching quantiles and compression. +This changes all-NaN results from an array of NaNs to an empty array, independent +of the requested quantile count or the order in which empty states are merged. +Other percentile arrays may retain their output shape when all states are empty. An empty TopN map is handled before full-map count adjustment, avoiding invalid counter changes when capacity is zero. diff --git a/regression-test/data/datatype_p0/agg_state/test_percentile_approx_array_empty.out b/regression-test/data/datatype_p0/agg_state/test_percentile_approx_array_empty.out new file mode 100644 index 00000000000000..2fb89dc88da0c1 --- /dev/null +++ b/regression-test/data/datatype_p0/agg_state/test_percentile_approx_array_empty.out @@ -0,0 +1,144 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !regular_phase_1 -- +0 [] [] +1 [7, 7] [7, 7] +2 [] [] + +-- !combine_phase_1 -- +0 [] +1 [7, 7] +2 [] + +-- !regular_phase_2 -- +0 [] [] +1 [7, 7] [7, 7] +2 [] [] + +-- !combine_phase_2 -- +0 [] +1 [7, 7] +2 [] + +-- !no_rows -- +[] + +-- !state_empty_merge_0 -- +[] + +-- !state_empty_union_0 -- +[] + +-- !state_later_0_false -- +[7, 7, 7] + +-- !state_later_0_true -- +[7, 7, 7] + +-- !state_association_0 -- +[7, 7, 7] + +-- !state_empty_merge_1 -- +[] + +-- !state_empty_union_1 -- +[] + +-- !state_later_1_false -- +[7, 7, 7] + +-- !state_later_1_true -- +[7, 7, 7] + +-- !state_association_1 -- +[7, 7, 7] + +-- !state_empty_merge_2 -- +[] + +-- !state_empty_union_2 -- +[] + +-- !state_later_2_false -- +[7, 7, 7] + +-- !state_later_2_true -- +[7, 7, 7] + +-- !state_association_2 -- +[7, 7, 7] + +-- !state_empty_merge_3 -- +[] + +-- !state_empty_union_3 -- +[] + +-- !state_later_3_false -- +[7, 7, 7] + +-- !state_later_3_true -- +[7, 7, 7] + +-- !state_association_3 -- +[7, 7, 7] + +-- !combine_empty_merge_0 -- +[] + +-- !combine_empty_union_0 -- +[] + +-- !combine_later_0_false -- +[7, 7, 7] + +-- !combine_later_0_true -- +[7, 7, 7] + +-- !combine_association_0 -- +[7, 7, 7] + +-- !combine_empty_merge_1 -- +[] + +-- !combine_empty_union_1 -- +[] + +-- !combine_later_1_false -- +[7, 7, 7] + +-- !combine_later_1_true -- +[7, 7, 7] + +-- !combine_association_1 -- +[7, 7, 7] + +-- !combine_empty_merge_2 -- +[] + +-- !combine_empty_union_2 -- +[] + +-- !combine_later_2_false -- +[7, 7, 7] + +-- !combine_later_2_true -- +[7, 7, 7] + +-- !combine_association_2 -- +[7, 7, 7] + +-- !combine_empty_merge_3 -- +[] + +-- !combine_empty_union_3 -- +[] + +-- !combine_later_3_false -- +[7, 7, 7] + +-- !combine_later_3_true -- +[7, 7, 7] + +-- !combine_association_3 -- +[7, 7, 7] + diff --git a/regression-test/suites/datatype_p0/agg_state/test_percentile_approx_array_empty.groovy b/regression-test/suites/datatype_p0/agg_state/test_percentile_approx_array_empty.groovy new file mode 100644 index 00000000000000..ee545b706b1a3b --- /dev/null +++ b/regression-test/suites/datatype_p0/agg_state/test_percentile_approx_array_empty.groovy @@ -0,0 +1,117 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("test_percentile_approx_array_empty") { + sql "set enable_agg_state=true" + sql "DROP TABLE IF EXISTS test_percentile_approx_array_empty_input" + sql """ + CREATE TABLE test_percentile_approx_array_empty_input (id INT, v DOUBLE NULL) + DUPLICATE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 2 + PROPERTIES ("replication_num" = "1") + """ + sql """ + INSERT INTO test_percentile_approx_array_empty_input VALUES + (0, cast('NaN' AS double)), (0, cast('NaN' AS double)), + (1, cast('NaN' AS double)), (1, 7), (2, NULL) + """ + for (def phase : [1, 2]) { + "order_qt_regular_phase_${phase}"(""" + SELECT /*+ SET_VAR(agg_phase=${phase}) */ id, + percentile_approx_array(v, [0.25, 0.75]), + percentile_approx_array(v, [0.25, 0.75], 2048) + FROM test_percentile_approx_array_empty_input GROUP BY id ORDER BY id + """) + "order_qt_combine_phase_${phase}"(""" + SELECT id, percentile_approx_array_merge(s) FROM ( + SELECT /*+ SET_VAR(agg_phase=${phase}) */ id, + percentile_approx_array_combine(v, [0.25, 0.75], 2048) s + FROM test_percentile_approx_array_empty_input GROUP BY id + ) states GROUP BY id ORDER BY id + """) + } + order_qt_no_rows """ + SELECT percentile_approx_array(v, [0.25, 0.75]) + FROM test_percentile_approx_array_empty_input WHERE id = 3 + """ + + for (def producer : ["state", "combine"]) { + def first = """ + SELECT percentile_approx_array_${producer}( + non_nullable(cast('NaN' AS double)), [0.25], 2048) s + """ + def second = """ + SELECT percentile_approx_array_${producer}( + non_nullable(cast('NaN' AS double)), [0.25, 0.75], 4096) s + """ + def noLevels = """ + SELECT percentile_approx_array_${producer}( + non_nullable(cast(7 AS double)), cast([] AS array), 6144) s + """ + def populated = """ + SELECT percentile_approx_array_${producer}( + non_nullable(cast(7 AS double)), [0.1, 0.5, 0.9], 10000) s + """ + int caseIndex = 0 + for (def pair : [[first, second], [second, first], [first, noLevels], [noLevels, first]]) { + def states = "${pair[0]} UNION ALL ${pair[1]}" + def united = "SELECT percentile_approx_array_union(s) s FROM (${states}) states" + "order_qt_${producer}_empty_merge_${caseIndex}"(""" + SELECT percentile_approx_array_merge(s) FROM (${states}) states + """) + "order_qt_${producer}_empty_union_${caseIndex}"(""" + SELECT percentile_approx_array_merge(s) FROM (${united}) united + """) + for (def contributorFirst : [false, true]) { + def branches = contributorFirst ? "${populated} UNION ALL ${united}" + : "${united} UNION ALL ${populated}" + "order_qt_${producer}_later_${caseIndex}_${contributorFirst}"(""" + SELECT percentile_approx_array_merge(s) FROM (${branches}) states + """) + } + // Group the contributor with the second empty state before merging the first. + "order_qt_${producer}_association_${caseIndex}"(""" + SELECT percentile_approx_array_merge(s) FROM ( + ${pair[0]} UNION ALL + SELECT percentile_approx_array_union(s) s FROM ( + ${pair[1]} UNION ALL ${populated} + ) inner_states + ) states + """) + caseIndex++ + } + + for (def parameters : ["[0.25], 10000", "[0.1, 0.5, 0.9], 2048"]) { + def incompatible = """ + SELECT percentile_approx_array_${producer}( + non_nullable(cast(8 AS double)), ${parameters}) s + """ + for (def pair : [[populated, incompatible], [incompatible, populated]]) { + for (def consumer : ["merge", "union"]) { + test { + sql """ + SELECT percentile_approx_array_${consumer}(s) FROM ( + ${pair[0]} UNION ALL ${pair[1]} + ) states + """ + exception "aggregate states have incompatible quantiles or compression" + } + } + } + } + } +}