From bbfe8a2ca7bcd52aee0f138b59db4ad96b0b623f Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Mon, 28 Mar 2022 15:28:17 +0000 Subject: [PATCH 1/2] fix possible loss of subcolumns in type Object --- src/DataTypes/DataTypeTuple.cpp | 2 +- .../0_stateless/01825_type_json_9.reference | 1 + tests/queries/0_stateless/01825_type_json_9.sql | 16 ++++++++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 tests/queries/0_stateless/01825_type_json_9.reference create mode 100644 tests/queries/0_stateless/01825_type_json_9.sql diff --git a/src/DataTypes/DataTypeTuple.cpp b/src/DataTypes/DataTypeTuple.cpp index a5e9868cf89..abf53a4baf1 100644 --- a/src/DataTypes/DataTypeTuple.cpp +++ b/src/DataTypes/DataTypeTuple.cpp @@ -206,7 +206,7 @@ bool DataTypeTuple::equals(const IDataType & rhs) const return false; for (size_t i = 0; i < size; ++i) - if (!elems[i]->equals(*rhs_tuple.elems[i])) + if (!elems[i]->equals(*rhs_tuple.elems[i]) || names[i] != rhs_tuple.names[i]) return false; return true; diff --git a/tests/queries/0_stateless/01825_type_json_9.reference b/tests/queries/0_stateless/01825_type_json_9.reference new file mode 100644 index 00000000000..a426b09a100 --- /dev/null +++ b/tests/queries/0_stateless/01825_type_json_9.reference @@ -0,0 +1 @@ +Tuple(foo Int8, k1 Int8, k2 Int8) diff --git a/tests/queries/0_stateless/01825_type_json_9.sql b/tests/queries/0_stateless/01825_type_json_9.sql new file mode 100644 index 00000000000..8fa4b335578 --- /dev/null +++ b/tests/queries/0_stateless/01825_type_json_9.sql @@ -0,0 +1,16 @@ +-- Tags: no-fasttest + +DROP TABLE IF EXISTS t_json; + +SET allow_experimental_object_type = 1; + +CREATE TABLE t_json(id UInt64, obj JSON) ENGINE = MergeTree ORDER BY id; + +INSERT INTO t_json format JSONEachRow {"id": 1, "obj": {"foo": 1, "k1": 2}}; +INSERT INTO t_json format JSONEachRow {"id": 2, "obj": {"foo": 1, "k2": 2}}; + +OPTIMIZE TABLE t_json FINAL; + +SELECT any(toTypeName(obj)) from t_json; + +DROP TABLE IF EXISTS t_json; From 6cbdc6af005f87e8b638c7c3f862cf1aea464a22 Mon Sep 17 00:00:00 2001 From: Anton Popov Date: Mon, 28 Mar 2022 18:44:53 +0000 Subject: [PATCH 2/2] remove obsolete parameter --- .../DataTypeLowCardinalityHelpers.cpp | 2 +- src/DataTypes/DataTypeTuple.cpp | 21 +++++++------------ src/DataTypes/DataTypeTuple.h | 6 +----- src/Functions/FunctionsConversion.h | 3 +-- src/Functions/tuple.cpp | 21 ++----------------- .../02008_tuple_to_name_value_pairs.sql | 2 +- 6 files changed, 13 insertions(+), 42 deletions(-) diff --git a/src/DataTypes/DataTypeLowCardinalityHelpers.cpp b/src/DataTypes/DataTypeLowCardinalityHelpers.cpp index 41ba81814d0..21ab25b6da3 100644 --- a/src/DataTypes/DataTypeLowCardinalityHelpers.cpp +++ b/src/DataTypes/DataTypeLowCardinalityHelpers.cpp @@ -36,7 +36,7 @@ DataTypePtr recursiveRemoveLowCardinality(const DataTypePtr & type) element = recursiveRemoveLowCardinality(element); if (tuple_type->haveExplicitNames()) - return std::make_shared(elements, tuple_type->getElementNames(), tuple_type->serializeNames()); + return std::make_shared(elements, tuple_type->getElementNames()); else return std::make_shared(elements); } diff --git a/src/DataTypes/DataTypeTuple.cpp b/src/DataTypes/DataTypeTuple.cpp index abf53a4baf1..908e0184b8d 100644 --- a/src/DataTypes/DataTypeTuple.cpp +++ b/src/DataTypes/DataTypeTuple.cpp @@ -64,8 +64,8 @@ static std::optional checkTupleNames(const Strings & names) return {}; } -DataTypeTuple::DataTypeTuple(const DataTypes & elems_, const Strings & names_, bool serialize_names_) - : elems(elems_), names(names_), have_explicit_names(true), serialize_names(serialize_names_) +DataTypeTuple::DataTypeTuple(const DataTypes & elems_, const Strings & names_) + : elems(elems_), names(names_), have_explicit_names(true) { size_t size = elems.size(); if (names.size() != size) @@ -75,11 +75,6 @@ DataTypeTuple::DataTypeTuple(const DataTypes & elems_, const Strings & names_, b throw std::move(*exception); } -bool DataTypeTuple::canBeCreatedWithNames(const Strings & names) -{ - return checkTupleNames(names) == std::nullopt; -} - std::string DataTypeTuple::doGetName() const { size_t size = elems.size(); @@ -91,7 +86,7 @@ std::string DataTypeTuple::doGetName() const if (i != 0) s << ", "; - if (have_explicit_names && serialize_names) + if (have_explicit_names) s << backQuoteIfNeed(names[i]) << ' '; s << elems[i]->getName(); @@ -265,31 +260,29 @@ size_t DataTypeTuple::getSizeOfValueInMemory() const SerializationPtr DataTypeTuple::doGetDefaultSerialization() const { SerializationTuple::ElementSerializations serializations(elems.size()); - bool use_explicit_names = have_explicit_names && serialize_names; for (size_t i = 0; i < elems.size(); ++i) { - String elem_name = use_explicit_names ? names[i] : toString(i + 1); + String elem_name = have_explicit_names ? names[i] : toString(i + 1); auto serialization = elems[i]->getDefaultSerialization(); serializations[i] = std::make_shared(serialization, elem_name); } - return std::make_shared(std::move(serializations), use_explicit_names); + return std::make_shared(std::move(serializations), have_explicit_names); } SerializationPtr DataTypeTuple::getSerialization(const SerializationInfo & info) const { SerializationTuple::ElementSerializations serializations(elems.size()); const auto & info_tuple = assert_cast(info); - bool use_explicit_names = have_explicit_names && serialize_names; for (size_t i = 0; i < elems.size(); ++i) { - String elem_name = use_explicit_names ? names[i] : toString(i + 1); + String elem_name = have_explicit_names ? names[i] : toString(i + 1); auto serialization = elems[i]->getSerialization(*info_tuple.getElementInfo(i)); serializations[i] = std::make_shared(serialization, elem_name); } - return std::make_shared(std::move(serializations), use_explicit_names); + return std::make_shared(std::move(serializations), have_explicit_names); } MutableSerializationInfoPtr DataTypeTuple::createSerializationInfo(const SerializationInfo::Settings & settings) const diff --git a/src/DataTypes/DataTypeTuple.h b/src/DataTypes/DataTypeTuple.h index db122aae5df..009a2284a0a 100644 --- a/src/DataTypes/DataTypeTuple.h +++ b/src/DataTypes/DataTypeTuple.h @@ -22,14 +22,11 @@ private: DataTypes elems; Strings names; bool have_explicit_names; - bool serialize_names = true; public: static constexpr bool is_parametric = true; explicit DataTypeTuple(const DataTypes & elems); - DataTypeTuple(const DataTypes & elems, const Strings & names, bool serialize_names_ = true); - - static bool canBeCreatedWithNames(const Strings & names); + DataTypeTuple(const DataTypes & elems, const Strings & names); TypeIndex getTypeId() const override { return TypeIndex::Tuple; } std::string doGetName() const override; @@ -66,7 +63,6 @@ public: String getNameByPosition(size_t i) const; bool haveExplicitNames() const { return have_explicit_names; } - bool serializeNames() const { return serialize_names; } }; } diff --git a/src/Functions/FunctionsConversion.h b/src/Functions/FunctionsConversion.h index e098378f51a..587efa9f217 100644 --- a/src/Functions/FunctionsConversion.h +++ b/src/Functions/FunctionsConversion.h @@ -2957,8 +2957,7 @@ private: /// For named tuples allow conversions for tuples with /// different sets of elements. If element exists in @to_type /// and doesn't exist in @to_type it will be filled by default values. - if (from_type->haveExplicitNames() && from_type->serializeNames() - && to_type->haveExplicitNames() && to_type->serializeNames()) + if (from_type->haveExplicitNames() && to_type->haveExplicitNames()) { const auto & from_names = from_type->getElementNames(); std::unordered_map from_positions; diff --git a/src/Functions/tuple.cpp b/src/Functions/tuple.cpp index 8e8b18e335d..6d5c53c0770 100644 --- a/src/Functions/tuple.cpp +++ b/src/Functions/tuple.cpp @@ -54,29 +54,12 @@ public: bool useDefaultImplementationForNulls() const override { return false; } bool useDefaultImplementationForConstants() const override { return true; } - DataTypePtr getReturnTypeImpl(const ColumnsWithTypeAndName & arguments) const override + DataTypePtr getReturnTypeImpl(const DataTypes & arguments) const override { if (arguments.empty()) throw Exception("Function " + getName() + " requires at least one argument.", ErrorCodes::NUMBER_OF_ARGUMENTS_DOESNT_MATCH); - DataTypes types; - Strings names; - - for (const auto & argument : arguments) - { - types.emplace_back(argument.type); - names.emplace_back(argument.name); - } - - /// Create named tuple if possible. We don't print tuple element names - /// because they are bad anyway -- aliases are not used, e.g. tuple(1 a) - /// will have element name '1' and not 'a'. If we ever change this, and - /// add the ability to access tuple elements by name, like tuple(1 a).a, - /// we should probably enable printing for better discoverability. - if (DataTypeTuple::canBeCreatedWithNames(names)) - return std::make_shared(types, names, false /*print names*/); - - return std::make_shared(types); + return std::make_shared(arguments); } ColumnPtr executeImpl(const ColumnsWithTypeAndName & arguments, const DataTypePtr &, size_t /*input_rows_count*/) const override diff --git a/tests/queries/0_stateless/02008_tuple_to_name_value_pairs.sql b/tests/queries/0_stateless/02008_tuple_to_name_value_pairs.sql index 9204975b579..59987a86590 100644 --- a/tests/queries/0_stateless/02008_tuple_to_name_value_pairs.sql +++ b/tests/queries/0_stateless/02008_tuple_to_name_value_pairs.sql @@ -4,7 +4,7 @@ DROP TABLE IF EXISTS test02008; CREATE TABLE test02008 ( col Tuple( a Tuple(key1 int, key2 int), - b Tuple(key1 int, key3 int) + b Tuple(key1 int, key2 int) ) ) ENGINE=Memory(); INSERT INTO test02008 VALUES (tuple(tuple(1, 2), tuple(3, 4)));