From 1d960e5c0b8325cb01d57a8efbcc83b6460704bc Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Mon, 22 Jul 2019 18:41:52 +0300 Subject: [PATCH 1/6] Fix CAST from LowCardinality(Nullable). --- dbms/src/Columns/ColumnLowCardinality.cpp | 27 +++++++++++++++++ dbms/src/Columns/ColumnLowCardinality.h | 4 +++ dbms/src/Functions/FunctionsConversion.h | 35 ++++++++++++++++------- 3 files changed, 56 insertions(+), 10 deletions(-) diff --git a/dbms/src/Columns/ColumnLowCardinality.cpp b/dbms/src/Columns/ColumnLowCardinality.cpp index f2a0ef1f626..47433942c51 100644 --- a/dbms/src/Columns/ColumnLowCardinality.cpp +++ b/dbms/src/Columns/ColumnLowCardinality.cpp @@ -352,6 +352,11 @@ ColumnPtr ColumnLowCardinality::countKeys() const return counter; } +bool ColumnLowCardinality::containsNull() const +{ + return getDictionary().nestedColumnIsNullable() && idx.containsDefault(); +} + ColumnLowCardinality::Index::Index() : positions(ColumnUInt8::create()), size_of_type(sizeof(UInt8)) {} @@ -605,6 +610,28 @@ void ColumnLowCardinality::Index::countKeys(ColumnUInt64::Container & counts) co callForType(std::move(counter), size_of_type); } +bool ColumnLowCardinality::Index::containsDefault() const +{ + bool contains = false; + + auto check_contains_default = [&](auto & x) + { + using CurIndexType = decltype(x); + auto & data = getPositionsData(); + for (auto pos : data) + { + if (pos == 0) + { + contains = true; + break; + } + } + }; + + callForType(std::move(check_contains_default), size_of_type); + return contains; +} + ColumnLowCardinality::Dictionary::Dictionary(MutableColumnPtr && column_unique_, bool is_shared) : column_unique(std::move(column_unique_)), shared(is_shared) diff --git a/dbms/src/Columns/ColumnLowCardinality.h b/dbms/src/Columns/ColumnLowCardinality.h index fc96fc97550..a6a129fbb09 100644 --- a/dbms/src/Columns/ColumnLowCardinality.h +++ b/dbms/src/Columns/ColumnLowCardinality.h @@ -194,6 +194,8 @@ public: ColumnPtr countKeys() const; + bool containsNull() const; + class Index { public: @@ -224,6 +226,8 @@ public: void countKeys(ColumnUInt64::Container & counts) const; + bool containsDefault() const; + private: WrappedPtr positions; size_t size_of_type = 0; diff --git a/dbms/src/Functions/FunctionsConversion.h b/dbms/src/Functions/FunctionsConversion.h index d8b8775a7a0..909cf5c2ef1 100644 --- a/dbms/src/Functions/FunctionsConversion.h +++ b/dbms/src/Functions/FunctionsConversion.h @@ -1896,11 +1896,17 @@ private: }; } - auto wrapper = prepareRemoveNullable(from_nested, to_nested); + bool skip_not_null_check = false; + + if (from_low_cardinality && from_nested->isNullable() && !to_nested->isNullable()) + /// Disable check for dictionary. Will check that column doesn't contain NULL in wrapper below. + skip_not_null_check = true; + + auto wrapper = prepareRemoveNullable(from_nested, to_nested, skip_not_null_check); if (!from_low_cardinality && !to_low_cardinality) return wrapper; - return [wrapper, from_low_cardinality, to_low_cardinality] + return [wrapper, from_low_cardinality, to_low_cardinality, skip_not_null_check] (Block & block, const ColumnNumbers & arguments, const size_t result, size_t input_rows_count) { auto & arg = block.getByPosition(arguments[0]); @@ -1925,6 +1931,11 @@ private: if (from_low_cardinality) { auto * col_low_cardinality = typeid_cast(prev_arg_col.get()); + + if (skip_not_null_check && col_low_cardinality->containsNull()) + throw Exception{"Cannot convert NULL value to non-Nullable type", + ErrorCodes::CANNOT_INSERT_NULL_IN_ORDINARY_COLUMN}; + arg.column = col_low_cardinality->getDictionary().getNestedColumn(); arg.type = from_low_cardinality->getDictionaryType(); @@ -1966,7 +1977,7 @@ private: }; } - WrapperType prepareRemoveNullable(const DataTypePtr & from_type, const DataTypePtr & to_type) const + WrapperType prepareRemoveNullable(const DataTypePtr & from_type, const DataTypePtr & to_type, bool skip_not_null_check) const { /// Determine whether pre-processing and/or post-processing must take place during conversion. @@ -2012,14 +2023,18 @@ private: Block tmp_block = createBlockWithNestedColumns(block, arguments, result); /// Check that all values are not-NULL. + /// Check can be skipped in case if LowCardinality dictionary is transformed. + /// In that case, correctness will be checked beforehand. + if (!skip_not_null_check) + { + const auto & col = block.getByPosition(arguments[0]).column; + const auto & nullable_col = static_cast(*col); + const auto & null_map = nullable_col.getNullMapData(); - const auto & col = block.getByPosition(arguments[0]).column; - const auto & nullable_col = static_cast(*col); - const auto & null_map = nullable_col.getNullMapData(); - - if (!memoryIsZero(null_map.data(), null_map.size())) - throw Exception{"Cannot convert NULL value to non-Nullable type", - ErrorCodes::CANNOT_INSERT_NULL_IN_ORDINARY_COLUMN}; + if (!memoryIsZero(null_map.data(), null_map.size())) + throw Exception{"Cannot convert NULL value to non-Nullable type", + ErrorCodes::CANNOT_INSERT_NULL_IN_ORDINARY_COLUMN}; + } wrapper(tmp_block, arguments, result, input_rows_count); block.getByPosition(result).column = tmp_block.getByPosition(result).column; From fa2610e0961500800ae2da14a7c0f98e03afd8c3 Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Mon, 22 Jul 2019 19:46:42 +0300 Subject: [PATCH 2/6] Fix CAST from LowCardinality(Nullable). --- dbms/src/Columns/ColumnLowCardinality.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Columns/ColumnLowCardinality.cpp b/dbms/src/Columns/ColumnLowCardinality.cpp index 47433942c51..1dbb3f8574f 100644 --- a/dbms/src/Columns/ColumnLowCardinality.cpp +++ b/dbms/src/Columns/ColumnLowCardinality.cpp @@ -614,7 +614,7 @@ bool ColumnLowCardinality::Index::containsDefault() const { bool contains = false; - auto check_contains_default = [&](auto & x) + auto check_contains_default = [&](auto x) { using CurIndexType = decltype(x); auto & data = getPositionsData(); From 1e35f8776099d77a338d2ab9729be83bb986a931 Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Tue, 23 Jul 2019 10:57:32 +0300 Subject: [PATCH 3/6] Fix CAST from LowCardinality(Nullable). --- dbms/src/Functions/FunctionsConversion.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Functions/FunctionsConversion.h b/dbms/src/Functions/FunctionsConversion.h index 909cf5c2ef1..7d1d277752f 100644 --- a/dbms/src/Functions/FunctionsConversion.h +++ b/dbms/src/Functions/FunctionsConversion.h @@ -2018,7 +2018,7 @@ private: { /// Conversion from Nullable to non-Nullable. - return [wrapper] (Block & block, const ColumnNumbers & arguments, const size_t result, size_t input_rows_count) + return [wrapper, skip_not_null_check] (Block & block, const ColumnNumbers & arguments, const size_t result, size_t input_rows_count) { Block tmp_block = createBlockWithNestedColumns(block, arguments, result); From eef0e08fbd543071104cda612f574504671da67d Mon Sep 17 00:00:00 2001 From: Nikolai Kochetov Date: Tue, 23 Jul 2019 13:04:26 +0300 Subject: [PATCH 4/6] Added test with LowCardinality(Nullable) to not-Nullable cast. --- .../0_stateless/00974_low_cardinality_cast.reference | 4 ++++ .../queries/0_stateless/00974_low_cardinality_cast.sql | 6 ++++++ 2 files changed, 10 insertions(+) create mode 100644 dbms/tests/queries/0_stateless/00974_low_cardinality_cast.reference create mode 100644 dbms/tests/queries/0_stateless/00974_low_cardinality_cast.sql diff --git a/dbms/tests/queries/0_stateless/00974_low_cardinality_cast.reference b/dbms/tests/queries/0_stateless/00974_low_cardinality_cast.reference new file mode 100644 index 00000000000..5c576f5dee6 --- /dev/null +++ b/dbms/tests/queries/0_stateless/00974_low_cardinality_cast.reference @@ -0,0 +1,4 @@ +Hello +\N +Hello +Hello diff --git a/dbms/tests/queries/0_stateless/00974_low_cardinality_cast.sql b/dbms/tests/queries/0_stateless/00974_low_cardinality_cast.sql new file mode 100644 index 00000000000..e369a8c169e --- /dev/null +++ b/dbms/tests/queries/0_stateless/00974_low_cardinality_cast.sql @@ -0,0 +1,6 @@ +SELECT CAST('Hello' AS LowCardinality(Nullable(String))); +SELECT CAST(Null AS LowCardinality(Nullable(String))); +SELECT CAST(CAST('Hello' AS LowCardinality(Nullable(String))) AS String); +SELECT CAST(CAST(Null AS LowCardinality(Nullable(String))) AS String); -- { serverError 349 } +SELECT CAST(CAST('Hello' AS Nullable(String)) AS String); +SELECT CAST(CAST(Null AS Nullable(String)) AS String); -- { serverError 349 } From db9f0cfba19093a2e11bb0fe76b8b7696158522a Mon Sep 17 00:00:00 2001 From: alesapin Date: Wed, 24 Jul 2019 14:10:34 +0300 Subject: [PATCH 5/6] Add empty text_log file --- dbms/tests/config/text_log.xml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 dbms/tests/config/text_log.xml diff --git a/dbms/tests/config/text_log.xml b/dbms/tests/config/text_log.xml new file mode 100644 index 00000000000..7e62283a83c --- /dev/null +++ b/dbms/tests/config/text_log.xml @@ -0,0 +1,2 @@ + + From 95fdc8a96b6eaa7f357746d37f03f81c3b456dd9 Mon Sep 17 00:00:00 2001 From: akuzm <36882414+akuzm@users.noreply.github.com> Date: Wed, 24 Jul 2019 15:04:18 +0300 Subject: [PATCH 6/6] Add a test case for #4402. (#6129) Turns out it's easy to reproduce using the `hits` table, so we can add a case to stateful tests. --- .../queries/1_stateful/00153_aggregate_arena_race.reference | 0 dbms/tests/queries/1_stateful/00153_aggregate_arena_race.sql | 2 ++ 2 files changed, 2 insertions(+) create mode 100644 dbms/tests/queries/1_stateful/00153_aggregate_arena_race.reference create mode 100644 dbms/tests/queries/1_stateful/00153_aggregate_arena_race.sql diff --git a/dbms/tests/queries/1_stateful/00153_aggregate_arena_race.reference b/dbms/tests/queries/1_stateful/00153_aggregate_arena_race.reference new file mode 100644 index 00000000000..e69de29bb2d diff --git a/dbms/tests/queries/1_stateful/00153_aggregate_arena_race.sql b/dbms/tests/queries/1_stateful/00153_aggregate_arena_race.sql new file mode 100644 index 00000000000..239b5ad0aa5 --- /dev/null +++ b/dbms/tests/queries/1_stateful/00153_aggregate_arena_race.sql @@ -0,0 +1,2 @@ +create temporary table dest00153 (`s` AggregateFunction(groupUniqArray, String)) engine Memory; +insert into dest00153 select groupUniqArrayState(RefererDomain) from test.hits group by URLDomain;