Merge pull request #12021 from ClickHouse/fix-if-fixed-string

Fix function if with FixedString arguments of different sizes
ClickHouse · Jul 5, 2020 · ab15c8d · ab15c8d
2 parents 0f1bfa5 + 918e979
commit ab15c8d
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 7 deletions.
diff --git a/src/Functions/if.cpp b/src/Functions/if.cpp
@@ -432,8 +432,7 @@ class FunctionIf : public FunctionIfBase</*null_is_false=*/false>
         const PaddedPODArray<UInt8> & cond_data = cond_col->getData();
         size_t rows = cond_data.size();
 
-        if ((col_then_fixed || col_then_const_fixed)
-            && (col_else_fixed || col_else_const_fixed))
+        if (isFixedString(block.getByPosition(result).type))
         {
             /// The result is FixedString.
 
@@ -448,16 +447,19 @@ class FunctionIf : public FunctionIfBase</*null_is_false=*/false>
             else if (col_then_const_fixed && col_else_fixed)
                 conditional(ConstSource<FixedStringSource>(*col_then_const_fixed), FixedStringSource(*col_else_fixed), sink, cond_data);
             else if (col_then_const_fixed && col_else_const_fixed)
-                conditional(ConstSource<FixedStringSource>(*col_then_const_fixed), ConstSource<FixedStringSource>(*col_else_const_fixed), sink, cond_data);
+                conditional(ConstSource<FixedStringSource>(*col_then_const_fixed),
+                            ConstSource<FixedStringSource>(*col_else_const_fixed), sink, cond_data);
+            else
+                return false;
 
             block.getByPosition(result).column = std::move(col_res_untyped);
             return true;
         }
 
-        if ((col_then || col_then_const || col_then_fixed || col_then_const_fixed)
-            && (col_else || col_else_const || col_else_fixed || col_else_const_fixed))
+        if (isString(block.getByPosition(result).type))
         {
             /// The result is String.
+
             auto col_res = ColumnString::create();
             auto sink = StringSink(*col_res, rows);
 
@@ -485,6 +487,17 @@ class FunctionIf : public FunctionIfBase</*null_is_false=*/false>
                 conditional(ConstSource<StringSource>(*col_then_const), ConstSource<FixedStringSource>(*col_else_const_fixed), sink, cond_data);
             else if (col_then_const_fixed && col_else_const)
                 conditional(ConstSource<FixedStringSource>(*col_then_const_fixed), ConstSource<StringSource>(*col_else_const), sink, cond_data);
+            else if (col_then_fixed && col_else_fixed)
+                conditional(FixedStringSource(*col_then_fixed), FixedStringSource(*col_else_fixed), sink, cond_data);
+            else if (col_then_fixed && col_else_const_fixed)
+                conditional(FixedStringSource(*col_then_fixed), ConstSource<FixedStringSource>(*col_else_const_fixed), sink, cond_data);
+            else if (col_then_const_fixed && col_else_fixed)
+                conditional(ConstSource<FixedStringSource>(*col_then_const_fixed), FixedStringSource(*col_else_fixed), sink, cond_data);
+            else if (col_then_const_fixed && col_else_const_fixed)
+                conditional(ConstSource<FixedStringSource>(*col_then_const_fixed),
+                            ConstSource<FixedStringSource>(*col_else_const_fixed), sink, cond_data);
+            else
+                return false;
 
             block.getByPosition(result).column = std::move(col_res);
             return true;
@@ -590,7 +603,8 @@ class FunctionIf : public FunctionIfBase</*null_is_false=*/false>
         return true;
     }
 
-    static void executeGeneric(const ColumnUInt8 * cond_col, Block & block, const ColumnNumbers & arguments, size_t result, size_t input_rows_count)
+    static void executeGeneric(
+        const ColumnUInt8 * cond_col, Block & block, const ColumnNumbers & arguments, size_t result, size_t input_rows_count)
     {
         /// Convert both columns to the common type (if needed).
 

diff --git a/src/Functions/multiIf.cpp b/src/Functions/multiIf.cpp
@@ -39,6 +39,7 @@ class FunctionMultiIf final : public FunctionIfBase</*null_is_false=*/true>
     bool isVariadic() const override { return true; }
     size_t getNumberOfArguments() const override { return 0; }
     bool useDefaultImplementationForNulls() const override { return false; }
+
     ColumnNumbers getArgumentsThatDontImplyNullableReturnType(size_t number_of_arguments) const override
     {
         ColumnNumbers args;
@@ -70,7 +71,6 @@ class FunctionMultiIf final : public FunctionIfBase</*null_is_false=*/true>
             throw Exception{"Invalid number of arguments for function " + getName(),
                 ErrorCodes::NUMBER_OF_ARGUMENTS_DOESNT_MATCH};
 
-
         for_conditions([&](const DataTypePtr & arg)
         {
             const IDataType * nested_type;

diff --git a/tests/queries/0_stateless/01355_if_fixed_string.reference b/tests/queries/0_stateless/01355_if_fixed_string.reference
@@ -0,0 +1,40 @@
+0\0\0\0\0	String
+1\0	String
+-2\0\0\0	String
+3\0	String
+-4\0\0\0	String
+5\0	String
+-6\0\0\0	String
+7\0	String
+-8\0\0\0	String
+9\0	String
+0\0	FixedString(2)
+1\0	FixedString(2)
+-2	FixedString(2)
+3\0	FixedString(2)
+-4	FixedString(2)
+5\0	FixedString(2)
+-6	FixedString(2)
+7\0	FixedString(2)
+-8	FixedString(2)
+9\0	FixedString(2)
+0	String
+1	String
+-2	String
+3	String
+-4	String
+5	String
+-6	String
+7	String
+-8	String
+9	String
+0\0	FixedString(2)
+1\0	FixedString(2)
+-2	FixedString(2)
+3\0	FixedString(2)
+-4	FixedString(2)
+5\0	FixedString(2)
+-6	FixedString(2)
+7\0	FixedString(2)
+-8	FixedString(2)
+9\0	FixedString(2)
diff --git a/tests/queries/0_stateless/01355_if_fixed_string.sql b/tests/queries/0_stateless/01355_if_fixed_string.sql
@@ -0,0 +1,5 @@
+SELECT if(number % 2, toFixedString(toString(number), 2), toFixedString(toString(-number), 5)) AS x, toTypeName(x) FROM system.numbers LIMIT 10;
+SELECT if(number % 2, toFixedString(toString(number), 2), toFixedString(toString(-number), 2)) AS x, toTypeName(x) FROM system.numbers LIMIT 10;
+
+SELECT multiIf(number % 2, toFixedString(toString(number), 2), toFixedString(toString(-number), 5)) AS x, toTypeName(x) FROM system.numbers LIMIT 10;
+SELECT multiIf(number % 2, toFixedString(toString(number), 2), toFixedString(toString(-number), 2)) AS x, toTypeName(x) FROM system.numbers LIMIT 10;