Fixed IN <Set> usage inside PK expressions.

Refactoring of type conversion functions convertFieldToType().

dbms/include/DB/Interpreters/Set.h

@@ -283,7 +283,7 @@ public:
 	  * node - это список значений: 1, 2, 3 или список tuple-ов: (1, 2), (3, 4), (5, 6).
 	  * create_ordered_set - создавать ли вектор упорядоченных элементов. Нужен для работы индекса
 	  */
-	void createFromAST(DataTypes & types, ASTPtr node, const Context & context, bool create_ordered_set);
+	void createFromAST(const DataTypes & types, ASTPtr node, const Context & context, bool create_ordered_set);
 
 	// Возвращает false, если превышено какое-нибудь ограничение, и больше не нужно вставлять.
 	bool insertFromBlock(const Block & block, bool create_ordered_set = false);

dbms/src/Core/FieldVisitors.cpp

@@ -117,6 +117,8 @@ DayNum_t stringToDate(const String & s)
 	DayNum_t date{};
 
 	readDateText(date, in);
+	if (!in.eof())
+		throw Exception("String is too long for Date: " + s);
 
 	return date;
 }

dbms/src/Interpreters/Set.cpp

@@ -279,7 +279,7 @@ static Field extractValueFromNode(ASTPtr & node, const IDataType & type, const C
 }
 
 
-void Set::createFromAST(DataTypes & types, ASTPtr node, const Context & context, bool create_ordered_set)
+void Set::createFromAST(const DataTypes & types, ASTPtr node, const Context & context, bool create_ordered_set)
 {
 	data_types = types;
 

dbms/src/Interpreters/convertFieldToType.cpp

@@ -9,6 +9,8 @@
 #include <DB/DataTypes/DataTypeDateTime.h>
 #include <DB/DataTypes/DataTypeEnum.h>
 
+#include <DB/Core/FieldVisitors.h>
+
 #include <DB/Interpreters/convertFieldToType.h>
 
 
@@ -74,19 +76,12 @@ Field convertFieldToType(const Field & src, const IDataType & type)
 
 		const bool is_date = typeid_cast<const DataTypeDate *>(&type);
 		bool is_datetime = false;
-		bool is_enum8 = false;
+		bool is_enum = false;
-		bool is_enum16 = false;
 
 		if (!is_date)
 			if (!(is_datetime = typeid_cast<const DataTypeDateTime *>(&type)))
-				if (!(is_enum8 = typeid_cast<const DataTypeEnum8 *>(&type)))
+				if (!(is_enum = dynamic_cast<const IDataTypeEnum *>(&type)))
-					if (!(is_enum16 = typeid_cast<const DataTypeEnum16 *>(&type)))
+					throw Exception{"Logical error: unknown numeric type " + type.getName(), ErrorCodes::LOGICAL_ERROR};
-						throw Exception{
-							"Logical error: unknown numeric type " + type.getName(),
-							ErrorCodes::LOGICAL_ERROR
-						};
-
-		const auto is_enum = is_enum8 || is_enum16;
 
 		/// Numeric values for Enums should not be used directly in IN section
 		if (src.getType() == Field::Types::UInt64 && !is_enum)
@@ -94,32 +89,21 @@ Field convertFieldToType(const Field & src, const IDataType & type)
 
 		if (src.getType() == Field::Types::String)
 		{
-			/// Возможность сравнивать даты и даты-с-временем со строкой.
-			const String & str = src.get<const String &>();
-			ReadBufferFromString in(str);
-
 			if (is_date)
 			{
-				DayNum_t date{};
+				/// Convert 'YYYY-MM-DD' Strings to Date
-				readDateText(date, in);
+				return UInt64(stringToDate(src.get<const String &>()));
-				if (!in.eof())
-					throw Exception("String is too long for Date: " + str);
-
-				return Field(UInt64(date));
 			}
 			else if (is_datetime)
 			{
-				time_t date_time{};
+				/// Convert 'YYYY-MM-DD hh:mm:ss' Strings to DateTime
-				readDateTimeText(date_time, in);
+				return stringToDateTime(src.get<const String &>());
-				if (!in.eof())
+			}
-					throw Exception("String is too long for DateTime: " + str);
+			else if (is_enum)
-
+			{
-				return Field(UInt64(date_time));
+				/// Convert String to Enum's value
+				return dynamic_cast<const IDataTypeEnum &>(type).castToValue(src);
 			}
-			else if (is_enum8)
-				return Field(UInt64(static_cast<const DataTypeEnum8 &>(type).getValue(str)));
-			else if (is_enum16)
-				return Field(UInt64(static_cast<const DataTypeEnum16 &>(type).getValue(str)));
 		}
 
 		throw Exception("Type mismatch in IN or VALUES section: " + type.getName() + " expected, "

dbms/src/Storages/MergeTree/PKCondition.cpp

@@ -11,6 +11,7 @@
 #include <DB/Parsers/ASTSet.h>
 #include <DB/Functions/FunctionFactory.h>
 #include <DB/Core/FieldVisitors.h>
+#include <DB/Interpreters/convertFieldToType.h>
 
 
 namespace DB
@@ -131,7 +132,7 @@ const PKCondition::AtomMap PKCondition::atom_map{
 	},
 	{
 		"in",
-		[] (RPNElement & out, const Field & value, ASTPtr & node)
+		[] (RPNElement & out, const Field &, ASTPtr & node)
 		{
 			out.function = RPNElement::FUNCTION_IN_SET;
 			out.in_function = node;
@@ -140,7 +141,7 @@ const PKCondition::AtomMap PKCondition::atom_map{
 	},
 	{
 		"notIn",
-		[] (RPNElement & out, const Field & value, ASTPtr & node)
+		[] (RPNElement & out, const Field &, ASTPtr & node)
 		{
 			out.function = RPNElement::FUNCTION_NOT_IN_SET;
 			out.in_function = node;
@@ -239,23 +240,26 @@ bool PKCondition::addCondition(const String & column, const Range & range)
 	return true;
 }
 
-/** Получить значение константного выражения.
+/** Computes value of constant expression and it data type.
-  * Вернуть false, если выражение не константно.
+  * Returns false, if expression isn't constant.
   */
-static bool getConstant(const ASTPtr & expr, Block & block_with_constants, Field & value)
+static bool getConstant(const ASTPtr & expr, Block & block_with_constants, Field & out_value, DataTypePtr & out_type)
 {
 	String column_name = expr->getColumnName();
 
-	if (const ASTLiteral * lit = typeid_cast<const ASTLiteral *>(&*expr))
+	if (const ASTLiteral * lit = typeid_cast<const ASTLiteral *>(expr.get()))
 	{
-		/// литерал
+		/// Simple literal
-		value = lit->value;
+		out_value = lit->value;
+		out_type = block_with_constants.getByName(column_name).type;
 		return true;
 	}
 	else if (block_with_constants.has(column_name) && block_with_constants.getByName(column_name).column->isConst())
 	{
-		/// выражение, вычислившееся в константу
+		/// An expression which is dependent on constants only
-		value = (*block_with_constants.getByName(column_name).column)[0];
+		const auto & expr_info = block_with_constants.getByName(column_name);
+		out_value = (*expr_info.column)[0];
+		out_type = expr_info.type;
 		return true;
 	}
 	else
@@ -362,46 +366,23 @@ bool PKCondition::isPrimaryKeyPossiblyWrappedByMonotonicFunctionsImpl(
 }
 
 
-/// NOTE: Keep in the mind that such behavior could be incompatible inside ordinary expression.
+static void castValueToType(const DataTypePtr & desired_type, Field & src_value, const DataTypePtr & src_type, const ASTPtr & node)
-/// TODO: Use common methods for types conversions.
-static bool tryCastValueToType(const DataTypePtr & desired_type, const DataTypePtr & src_type, Field & src_value)
 {
 	if (desired_type->getName() == src_type->getName())
-		return true;
+		return;
 
-	/// Try to correct type of constant for correct comparison
 	try
 	{
-		/// Convert String to Enum's value
+		/// NOTE: We don't need accurate info about src_type at this moment
-		if (auto data_type_enum = dynamic_cast<const IDataTypeEnum *>(desired_type.get()))
+		src_value = convertFieldToType(src_value, *desired_type);
-		{
-			src_value = data_type_enum->castToValue(src_value);
-		}
-		/// Convert 'YYYY-MM-DD' Strings to Date
-		else if (typeid_cast<const DataTypeDate *>(desired_type.get()) && typeid_cast<const DataTypeString *>(src_type.get()))
-		{
-			src_value = UInt64(stringToDate(src_value.safeGet<String>()));
-		}
-		/// Convert 'YYYY-MM-DD hh:mm:ss' Strings to DateTime
-		else if (typeid_cast<const DataTypeDateTime *>(desired_type.get()) && typeid_cast<const DataTypeString *>(src_type.get()))
-		{
-			src_value = stringToDateTime(src_value.safeGet<String>());
-		}
-		else if (desired_type->behavesAsNumber() && src_type->behavesAsNumber())
-		{
-			/// Ok, numeric types are almost mutually convertible
-		}
-		else
-		{
-			return false;
-		}
 	}
 	catch (...)
 	{
-		return false;
+		throw Exception("Primary key expression contains comparison between inconvertible types: " +
+			desired_type->getName() + " and " + src_type->getName() +
+			" inside " + DB::toString(node->range),
+			ErrorCodes::BAD_TYPE_OF_FIELD);
 	}
-
-	return true;
 }
 
 
@@ -411,8 +392,9 @@ bool PKCondition::atomFromAST(ASTPtr & node, const Context & context, Block & bl
 	  *  либо он же, завёрнутый в цепочку возможно-монотонных функций,
 	  *  либо константное выражение - число.
 	  */
-	Field value;
+	Field const_value;
-	if (const ASTFunction * func = typeid_cast<const ASTFunction *>(&*node))
+	DataTypePtr const_type;
+	if (const ASTFunction * func = typeid_cast<const ASTFunction *>(node.get()))
 	{
 		const ASTs & args = typeid_cast<const ASTExpressionList &>(*func->arguments).children;
 
@@ -423,13 +405,14 @@ bool PKCondition::atomFromAST(ASTPtr & node, const Context & context, Block & bl
 		size_t key_arg_pos;			/// Position of argument with primary key column (non-const argument)
 		size_t key_column_num;		/// Number of a primary key column (inside sort_descr array)
 		RPNElement::MonotonicFunctionsChain chain;
+		bool is_set_const = false;
 
-		if (getConstant(args[1], block_with_constants, value)
+		if (getConstant(args[1], block_with_constants, const_value, const_type)
 			&& isPrimaryKeyPossiblyWrappedByMonotonicFunctions(args[0], context, key_column_num, key_expr_type, chain))
 		{
 			key_arg_pos = 0;
 		}
-		else if (getConstant(args[0], block_with_constants, value)
+		else if (getConstant(args[0], block_with_constants, const_value, const_type)
 			&& isPrimaryKeyPossiblyWrappedByMonotonicFunctions(args[1], context, key_column_num, key_expr_type, chain))
 		{
 			key_arg_pos = 1;
@@ -438,6 +421,7 @@ bool PKCondition::atomFromAST(ASTPtr & node, const Context & context, Block & bl
 			&& isPrimaryKeyPossiblyWrappedByMonotonicFunctions(args[0], context, key_column_num, key_expr_type, chain))
 		{
 			key_arg_pos = 0;
+			is_set_const = true;
 		}
 		else
 			return false;
@@ -469,26 +453,19 @@ bool PKCondition::atomFromAST(ASTPtr & node, const Context & context, Block & bl
 		if (atom_it == std::end(atom_map))
 			return false;
 
-		const DataTypePtr & const_type = block_with_constants.getByName(args[1 - key_arg_pos]->getColumnName()).type;
+		if (!is_set_const) /// Set args are already casted inside Set::createFromAST
+			castValueToType(key_expr_type, const_value, const_type, node);
 
-		if (!tryCastValueToType(key_expr_type, const_type, value))
+		return atom_it->second(out, const_value, node);
-		{
-			throw Exception("Primary key expression contains comparison between inconvertible types: " +
-				key_expr_type->getName() + " and " + const_type->getName() +
-				" inside " + DB::toString(func->range),
-				ErrorCodes::BAD_TYPE_OF_FIELD);
-		}
-
-		return atom_it->second(out, value, node);
 	}
-	else if (getConstant(node, block_with_constants, value))	/// Для случаев, когда написано, например, WHERE 0 AND something
+	else if (getConstant(node, block_with_constants, const_value, const_type))	/// Для случаев, когда написано, например, WHERE 0 AND something
 	{
-		if (value.getType() == Field::Types::UInt64
+		if (const_value.getType() == Field::Types::UInt64
-			|| value.getType() == Field::Types::Int64
+			|| const_value.getType() == Field::Types::Int64
-			|| value.getType() == Field::Types::Float64)
+			|| const_value.getType() == Field::Types::Float64)
 		{
 			/// Ноль во всех типах представлен в памяти так же, как в UInt64.
-			out.function = value.get<UInt64>()
+			out.function = const_value.get<UInt64>()
 				? RPNElement::ALWAYS_TRUE
 				: RPNElement::ALWAYS_FALSE;
 

dbms/tests/queries/0_stateless/00386_enum_in_pk.reference

@@ -20,5 +20,7 @@
 3447905173014179293
 3051197876967004596
 3051197876967004596
+3051197876967004596
+3051197876967004596
 463667963421364848
 463667963421364848

dbms/tests/queries/0_stateless/00386_enum_in_pk.sql

@@ -35,5 +35,8 @@ SELECT cityHash64(groupArray(d)) FROM test.enum_pk WHERE 1 = 1;
 SELECT cityHash64(groupArray(x)) FROM test.enum_pk WHERE (x = '0' OR x = '1');
 SELECT cityHash64(groupArray(d)) FROM test.enum_pk WHERE (d = '0' OR d = '1');
 
+SELECT cityHash64(groupArray(x)) FROM test.enum_pk WHERE x IN ('0', '1');
+SELECT cityHash64(groupArray(d)) FROM test.enum_pk WHERE d IN ('0', '1');
+
 SELECT cityHash64(groupArray(x)) FROM test.enum_pk WHERE (x != '0' AND x != '1');
 SELECT cityHash64(groupArray(d)) FROM test.enum_pk WHERE (d != '0' AND d != '1');