From 5efbae615c4c5d8e67a50e1be86a1c1385a8cc68 Mon Sep 17 00:00:00 2001 From: avogar Date: Mon, 11 Nov 2024 15:07:38 +0000 Subject: [PATCH 1/3] Forbid Dynamic type in min/max functions to avoid confusion --- .../AggregateFunctionsArgMinArgMax.cpp | 8 ++++++++ src/AggregateFunctions/AggregateFunctionsMinMax.cpp | 8 ++++++++ .../AggregateFunctionCombinatorsArgMinArgMax.cpp | 8 ++++++++ src/Storages/MergeTree/MergeTreeIndexMinMax.cpp | 8 ++++++++ .../0_stateless/03271_dynamic_in_min_max.reference | 0 tests/queries/0_stateless/03271_dynamic_in_min_max.sql | 9 +++++++++ 6 files changed, 41 insertions(+) create mode 100644 tests/queries/0_stateless/03271_dynamic_in_min_max.reference create mode 100644 tests/queries/0_stateless/03271_dynamic_in_min_max.sql diff --git a/src/AggregateFunctions/AggregateFunctionsArgMinArgMax.cpp b/src/AggregateFunctions/AggregateFunctionsArgMinArgMax.cpp index 9608ca26f37..cc72a26af16 100644 --- a/src/AggregateFunctions/AggregateFunctionsArgMinArgMax.cpp +++ b/src/AggregateFunctions/AggregateFunctionsArgMinArgMax.cpp @@ -79,6 +79,14 @@ public: "Illegal type {} of second argument of aggregate function {} because the values of that data type are not comparable", type_val->getName(), getName()); + + if (isDynamic(this->type_val) || isVariant(this->type_val)) + throw Exception( + ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, + "Illegal type {} of argument of aggregate function {} because the column of that type can contain values with different " + "data types. Consider using typed subcolumns or cast column to a specific data type", + this->type_val->getName(), + getName()); } void create(AggregateDataPtr __restrict place) const override /// NOLINT diff --git a/src/AggregateFunctions/AggregateFunctionsMinMax.cpp b/src/AggregateFunctions/AggregateFunctionsMinMax.cpp index 5fa9a4ff5d1..0c21be574c4 100644 --- a/src/AggregateFunctions/AggregateFunctionsMinMax.cpp +++ b/src/AggregateFunctions/AggregateFunctionsMinMax.cpp @@ -35,6 +35,14 @@ public: "Illegal type {} of argument of aggregate function {} because the values of that data type are not comparable", this->result_type->getName(), getName()); + + if (isDynamic(this->result_type) || isVariant(this->result_type)) + throw Exception( + ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, + "Illegal type {} of argument of aggregate function {} because the column of that type can contain values with different " + "data types. Consider using typed subcolumns or cast column to a specific data type", + this->result_type->getName(), + getName()); } String getName() const override diff --git a/src/AggregateFunctions/Combinators/AggregateFunctionCombinatorsArgMinArgMax.cpp b/src/AggregateFunctions/Combinators/AggregateFunctionCombinatorsArgMinArgMax.cpp index a1716f18725..fc8b5b6d4cd 100644 --- a/src/AggregateFunctions/Combinators/AggregateFunctionCombinatorsArgMinArgMax.cpp +++ b/src/AggregateFunctions/Combinators/AggregateFunctionCombinatorsArgMinArgMax.cpp @@ -63,6 +63,14 @@ public: "Illegal type {} for combinator {} because the values of that data type are not comparable", arguments[key_col]->getName(), getName()); + + if (isDynamic(arguments[key_col]) || isVariant(arguments[key_col])) + throw Exception( + ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT, + "Illegal type {} of argument of aggregate function {} because the column of that type can contain values with different " + "data types. Consider using typed subcolumns or cast column to a specific data type", + arguments[key_col]->getName(), + getName()); } String getName() const override diff --git a/src/Storages/MergeTree/MergeTreeIndexMinMax.cpp b/src/Storages/MergeTree/MergeTreeIndexMinMax.cpp index 07fd873a000..c859053b976 100644 --- a/src/Storages/MergeTree/MergeTreeIndexMinMax.cpp +++ b/src/Storages/MergeTree/MergeTreeIndexMinMax.cpp @@ -231,6 +231,14 @@ void minmaxIndexValidator(const IndexDescription & index, bool attach) "Data type of argument for minmax index must be comparable, got {} type for column {} instead", column.type->getName(), column.name); } + + if (isDynamic(column.type) || isVariant(column.type)) + { + throw Exception(ErrorCodes::BAD_ARGUMENTS, + "{} data type of column {} is not allowed in minmax index because the column of that type can contain values with different data " + "types. Consider using typed subcolumns or cast column to a specific data type", + column.type->getName(), column.name); + } } } diff --git a/tests/queries/0_stateless/03271_dynamic_in_min_max.reference b/tests/queries/0_stateless/03271_dynamic_in_min_max.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/03271_dynamic_in_min_max.sql b/tests/queries/0_stateless/03271_dynamic_in_min_max.sql new file mode 100644 index 00000000000..77ad9f0dc93 --- /dev/null +++ b/tests/queries/0_stateless/03271_dynamic_in_min_max.sql @@ -0,0 +1,9 @@ +set allow_experimental_dynamic_type=1; +select max(number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select min(number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select argMax(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select argMin(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select anyArgMax(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select anyArgMin(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +create table test (d Dynamic, index idx d type minmax); -- {serverError BAD_ARGUMENTS} + From b62aac446e55bea82278a72a64d64b6001378993 Mon Sep 17 00:00:00 2001 From: avogar Date: Mon, 11 Nov 2024 15:11:55 +0000 Subject: [PATCH 2/3] Better tests --- .../03271_dynamic_variant_in_min_max.reference | 0 .../03271_dynamic_variant_in_min_max.sql | 18 ++++++++++++++++++ 2 files changed, 18 insertions(+) create mode 100644 tests/queries/0_stateless/03271_dynamic_variant_in_min_max.reference create mode 100644 tests/queries/0_stateless/03271_dynamic_variant_in_min_max.sql diff --git a/tests/queries/0_stateless/03271_dynamic_variant_in_min_max.reference b/tests/queries/0_stateless/03271_dynamic_variant_in_min_max.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/queries/0_stateless/03271_dynamic_variant_in_min_max.sql b/tests/queries/0_stateless/03271_dynamic_variant_in_min_max.sql new file mode 100644 index 00000000000..a7fea941548 --- /dev/null +++ b/tests/queries/0_stateless/03271_dynamic_variant_in_min_max.sql @@ -0,0 +1,18 @@ +set allow_experimental_dynamic_type=1; +select max(number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select min(number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select argMax(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select argMin(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select anyArgMax(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select anyArgMin(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +create table test (d Dynamic, index idx d type minmax); -- {serverError BAD_ARGUMENTS} + +set allow_experimental_variant_type=1; +select max(number::Variant(UInt64)) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select min(number::Variant(UInt64)) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select argMax(number, number::Variant(UInt64)) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select argMin(number, number::Variant(UInt64)) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select anyArgMax(number, number::Variant(UInt64)) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +select anyArgMin(number, number::Variant(UInt64)) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} +create table test (d Variant(UInt64), index idx d type minmax); -- {serverError BAD_ARGUMENTS} + From 75cbf0ca9a832b4b3d8950f544ca770ecfd27d24 Mon Sep 17 00:00:00 2001 From: avogar Date: Mon, 11 Nov 2024 15:12:30 +0000 Subject: [PATCH 3/3] Remove old test --- .../0_stateless/03271_dynamic_in_min_max.reference | 0 tests/queries/0_stateless/03271_dynamic_in_min_max.sql | 9 --------- 2 files changed, 9 deletions(-) delete mode 100644 tests/queries/0_stateless/03271_dynamic_in_min_max.reference delete mode 100644 tests/queries/0_stateless/03271_dynamic_in_min_max.sql diff --git a/tests/queries/0_stateless/03271_dynamic_in_min_max.reference b/tests/queries/0_stateless/03271_dynamic_in_min_max.reference deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/tests/queries/0_stateless/03271_dynamic_in_min_max.sql b/tests/queries/0_stateless/03271_dynamic_in_min_max.sql deleted file mode 100644 index 77ad9f0dc93..00000000000 --- a/tests/queries/0_stateless/03271_dynamic_in_min_max.sql +++ /dev/null @@ -1,9 +0,0 @@ -set allow_experimental_dynamic_type=1; -select max(number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} -select min(number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} -select argMax(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} -select argMin(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} -select anyArgMax(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} -select anyArgMin(number, number::Dynamic) from numbers(10); -- {serverError ILLEGAL_TYPE_OF_ARGUMENT} -create table test (d Dynamic, index idx d type minmax); -- {serverError BAD_ARGUMENTS} -