From 54510bf393e2f0b05dfa27b05f0486c78f55adb6 Mon Sep 17 00:00:00 2001 From: Ti Chi Robot Date: Tue, 12 Apr 2022 20:28:36 +0800 Subject: [PATCH] fix throw `Unexpected type of column: Nullable(Nothing)` in LogicalOp (#3371) (#3378) close pingcap/tiflash#3351 --- dbms/src/Functions/FunctionsLogical.h | 252 +++++++++------------- tests/fullstack-test/expr/logical_op.test | 44 ++++ 2 files changed, 151 insertions(+), 145 deletions(-) diff --git a/dbms/src/Functions/FunctionsLogical.h b/dbms/src/Functions/FunctionsLogical.h index 4d8d7b71548..82aa25884eb 100644 --- a/dbms/src/Functions/FunctionsLogical.h +++ b/dbms/src/Functions/FunctionsLogical.h @@ -1,15 +1,16 @@ #pragma once -#include -#include #include #include -#include +#include #include -#include -#include -#include +#include +#include #include +#include +#include +#include + #include @@ -18,7 +19,7 @@ namespace DB namespace ErrorCodes { - extern const int NUMBER_OF_ARGUMENTS_DOESNT_MATCH; +extern const int NUMBER_OF_ARGUMENTS_DOESNT_MATCH; } /** Behaviour in presence of NULLs: @@ -34,20 +35,14 @@ namespace ErrorCodes struct AndImpl { - static inline bool isSaturable() - { - return true; - } + static inline bool isSaturable() { return true; } static inline bool resNotNull(const Field & value) { return !value.isNull() && applyVisitor(FieldVisitorConvertToNumber(), value) == 0; } - static inline bool resNotNull(UInt8 value, UInt8 is_null) - { - return !is_null && !value; - } + static inline bool resNotNull(UInt8 value, UInt8 is_null) { return !is_null && !value; } static inline void adjustForNullValue(UInt8 & value, UInt8 & is_null) { @@ -55,39 +50,23 @@ struct AndImpl value = false; } - static inline bool isSaturatedValue(bool a) - { - return !a; - } - - static inline bool apply(bool a, bool b) - { - return a && b; - } + static inline bool isSaturatedValue(bool a) { return !a; } + static inline bool apply(bool a, bool b) { return a && b; } }; struct OrImpl { - static inline bool isSaturable() - { - return true; - } + static inline bool isSaturable() { return true; } - static inline bool isSaturatedValue(bool a) - { - return a; - } + static inline bool isSaturatedValue(bool a) { return a; } static inline bool resNotNull(const Field & value) { return !value.isNull() && applyVisitor(FieldVisitorConvertToNumber(), value) == 1; } - static inline bool resNotNull(UInt8 value, UInt8 is_null) - { - return !is_null && value; - } + static inline bool resNotNull(UInt8 value, UInt8 is_null) { return !is_null && value; } static inline void adjustForNullValue(UInt8 & value, UInt8 & is_null) { @@ -95,43 +74,22 @@ struct OrImpl value = true; } - static inline bool apply(bool a, bool b) - { - return a || b; - } - + static inline bool apply(bool a, bool b) { return a || b; } }; struct XorImpl { - static inline bool isSaturable() - { - return false; - } + static inline bool isSaturable() { return false; } - static inline bool isSaturatedValue(bool) - { - return false; - } + static inline bool isSaturatedValue(bool) { return false; } - static inline bool resNotNull(const Field & ) - { - return true; - } + static inline bool resNotNull(const Field &) { return true; } - static inline bool resNotNull(UInt8 , UInt8 ) - { - return true; - } + static inline bool resNotNull(UInt8, UInt8) { return true; } - static inline void adjustForNullValue(UInt8 & , UInt8 & ) - { - } + static inline void adjustForNullValue(UInt8 &, UInt8 &) {} - static inline bool apply(bool a, bool b) - { - return a != b; - } + static inline bool apply(bool a, bool b) { return a != b; } }; template @@ -139,10 +97,7 @@ struct NotImpl { using ResultType = UInt8; - static inline UInt8 apply(A a) - { - return !a; - } + static inline UInt8 apply(A a) { return !a; } }; @@ -176,8 +131,7 @@ struct AssociativeOperationImpl AssociativeOperationImpl continuation; /// Remembers the last N columns from `in`. - AssociativeOperationImpl(UInt8ColumnPtrs & in) - : vec(in[in.size() - N]->getData()), continuation(in) {} + AssociativeOperationImpl(UInt8ColumnPtrs & in) : vec(in[in.size() - N]->getData()), continuation(in) {} /// Returns a combination of values in the i-th row of all columns stored in the constructor. inline UInt8 apply(size_t i) const @@ -204,13 +158,9 @@ struct AssociativeOperationImpl const UInt8Container & vec; - AssociativeOperationImpl(UInt8ColumnPtrs & in) - : vec(in[in.size() - 1]->getData()) {} + AssociativeOperationImpl(UInt8ColumnPtrs & in) : vec(in[in.size() - 1]->getData()) {} - inline UInt8 apply(size_t i) const - { - return vec[i]; - } + inline UInt8 apply(size_t i) const { return vec[i]; } }; @@ -258,7 +208,7 @@ class FunctionAnyArityLogical : public IFunction } template - bool convertTypeToUInt8(const IColumn * column, UInt8Container & res, UInt8Container & res_not_null) + bool convertTypeToUInt8(const IColumn * column, UInt8Container & res, UInt8Container & res_not_null) const { auto col = checkAndGetColumn>(column); if (!col) @@ -275,9 +225,29 @@ class FunctionAnyArityLogical : public IFunction return true; } + bool convertOnlyNullToUInt8( + const IColumn * column, UInt8Container & res, UInt8Container & res_not_null, UInt8Container & input_has_null) const + { + if (!column->onlyNull()) + return false; + + size_t n = res.size(); + for (size_t i = 0; i < n; ++i) + { + res[i] = false; + if constexpr (special_impl_for_nulls) + { + res_not_null[i] |= Impl::resNotNull(res[i], true); + input_has_null[i] |= true; + } + } + + return true; + } + template - bool convertNullableTypeToUInt8(const IColumn * column, UInt8Container & res, UInt8Container & res_not_null, - UInt8Container & input_has_null) + bool convertNullableTypeToUInt8( + const IColumn * column, UInt8Container & res, UInt8Container & res_not_null, UInt8Container & input_has_null) const { auto col_nullable = checkAndGetColumn(column); @@ -302,36 +272,29 @@ class FunctionAnyArityLogical : public IFunction return true; } - void convertToUInt8(const IColumn * column, UInt8Container & res, UInt8Container & res_not_null, - UInt8Container & input_has_null) - { - if (!convertTypeToUInt8(column, res, res_not_null) && - !convertTypeToUInt8(column, res, res_not_null) && - !convertTypeToUInt8(column, res, res_not_null) && - !convertTypeToUInt8(column, res, res_not_null) && - !convertTypeToUInt8(column, res, res_not_null) && - !convertTypeToUInt8(column, res, res_not_null) && - !convertTypeToUInt8(column, res, res_not_null) && - !convertTypeToUInt8(column, res, res_not_null) && - !convertTypeToUInt8(column, res, res_not_null) && - !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) && - !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) && - !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) && - !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) && - !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) && - !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) && - !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) && - !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) && - !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) && - !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null)) + void convertToUInt8(const IColumn * column, UInt8Container & res, UInt8Container & res_not_null, UInt8Container & input_has_null) const + { + if (!convertTypeToUInt8(column, res, res_not_null) && !convertTypeToUInt8(column, res, res_not_null) + && !convertTypeToUInt8(column, res, res_not_null) && !convertTypeToUInt8(column, res, res_not_null) + && !convertTypeToUInt8(column, res, res_not_null) && !convertTypeToUInt8(column, res, res_not_null) + && !convertTypeToUInt8(column, res, res_not_null) && !convertTypeToUInt8(column, res, res_not_null) + && !convertTypeToUInt8(column, res, res_not_null) + && !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) + && !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) + && !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) + && !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) + && !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) + && !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) + && !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) + && !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) + && !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) + && !convertNullableTypeToUInt8(column, res, res_not_null, input_has_null) + && !convertOnlyNullToUInt8(column, res, res_not_null, input_has_null)) throw Exception("Unexpected type of column: " + column->getName(), ErrorCodes::ILLEGAL_COLUMN); } public: - String getName() const override - { - return name; - } + String getName() const override { return name; } bool isVariadic() const override { return true; } size_t getNumberOfArguments() const override { return 0; } @@ -342,8 +305,8 @@ class FunctionAnyArityLogical : public IFunction DataTypePtr getReturnTypeImpl(const DataTypes & arguments) const override { if (arguments.size() < 2) - throw Exception("Number of arguments for function " + getName() + " doesn't match: passed " - + toString(arguments.size()) + ", should be at least 2.", + throw Exception("Number of arguments for function " + getName() + " doesn't match: passed " + toString(arguments.size()) + + ", should be at least 2.", ErrorCodes::NUMBER_OF_ARGUMENTS_DOESNT_MATCH); bool has_nullable_input_column = false; @@ -351,11 +314,10 @@ class FunctionAnyArityLogical : public IFunction { has_nullable_input_column |= arguments[i]->isNullable(); if (!(arguments[i]->isNumber() - || (special_impl_for_nulls && (arguments[i]->onlyNull() || removeNullable(arguments[i])->isNumber())))) - throw Exception("Illegal type (" - + arguments[i]->getName() - + ") of " + toString(i + 1) + " argument of function " + getName(), - ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); + || (special_impl_for_nulls && (arguments[i]->onlyNull() || removeNullable(arguments[i])->isNumber())))) + throw Exception( + "Illegal type (" + arguments[i]->getName() + ") of " + toString(i + 1) + " argument of function " + getName(), + ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); } if (has_nullable_input_column) @@ -396,12 +358,11 @@ class FunctionAnyArityLogical : public IFunction if (const_val_input_has_null && const_val_res_not_null) Impl::adjustForNullValue(const_val, const_val_input_has_null); if (const_val_input_has_null) - block.getByPosition(result).column = - block.getByPosition(result).type->createColumnConst(rows,Null()); + block.getByPosition(result).column = block.getByPosition(result).type->createColumnConst(rows, Null()); else - block.getByPosition(result).column = has_nullable_input_column ? makeNullable( - DataTypeUInt8().createColumnConst(rows, toField(const_val))) : - DataTypeUInt8().createColumnConst(rows, toField(const_val)); + block.getByPosition(result).column = has_nullable_input_column + ? makeNullable(DataTypeUInt8().createColumnConst(rows, toField(const_val))) + : DataTypeUInt8().createColumnConst(rows, toField(const_val)); } return; } @@ -434,8 +395,8 @@ class FunctionAnyArityLogical : public IFunction vec_res.resize(rows); if constexpr (special_impl_for_nulls) { - vec_input_has_null.assign(rows, (UInt8) 0); - vec_res_not_null.assign(rows, (UInt8) 0); + vec_input_has_null.assign(rows, (UInt8)0); + vec_res_not_null.assign(rows, (UInt8)0); } } @@ -494,8 +455,7 @@ class FunctionAnyArityLogical : public IFunction if (vec_input_has_null[i] && vec_res_not_null[i]) Impl::adjustForNullValue(vec_res[i], vec_input_has_null[i]); } - block.getByPosition(result).column = ColumnNullable::create(std::move(col_res), - std::move(col_input_has_null)); + block.getByPosition(result).column = ColumnNullable::create(std::move(col_res), std::move(col_input_has_null)); } else block.getByPosition(result).column = std::move(col_res); @@ -531,19 +491,14 @@ class FunctionUnaryLogical : public IFunction } public: - String getName() const override - { - return name; - } + String getName() const override { return name; } size_t getNumberOfArguments() const override { return 1; } DataTypePtr getReturnTypeImpl(const DataTypes & arguments) const override { if (!arguments[0]->isNumber()) - throw Exception("Illegal type (" - + arguments[0]->getName() - + ") of argument of function " + getName(), + throw Exception("Illegal type (" + arguments[0]->getName() + ") of argument of function " + getName(), ErrorCodes::ILLEGAL_TYPE_OF_ARGUMENT); return std::make_shared(); @@ -553,31 +508,38 @@ class FunctionUnaryLogical : public IFunction void executeImpl(Block & block, const ColumnNumbers & arguments, size_t result) override { - if (!( executeType(block, arguments, result) - || executeType(block, arguments, result) - || executeType(block, arguments, result) - || executeType(block, arguments, result) - || executeType(block, arguments, result) - || executeType(block, arguments, result) - || executeType(block, arguments, result) - || executeType(block, arguments, result) - || executeType(block, arguments, result) - || executeType(block, arguments, result))) - throw Exception("Illegal column " + block.getByPosition(arguments[0]).column->getName() - + " of argument of function " + getName(), + if (!(executeType(block, arguments, result) || executeType(block, arguments, result) + || executeType(block, arguments, result) || executeType(block, arguments, result) + || executeType(block, arguments, result) || executeType(block, arguments, result) + || executeType(block, arguments, result) || executeType(block, arguments, result) + || executeType(block, arguments, result) || executeType(block, arguments, result))) + throw Exception( + "Illegal column " + block.getByPosition(arguments[0]).column->getName() + " of argument of function " + getName(), ErrorCodes::ILLEGAL_COLUMN); } }; -struct NameAnd { static constexpr auto name = "and"; }; -struct NameOr { static constexpr auto name = "or"; }; -struct NameXor { static constexpr auto name = "xor"; }; -struct NameNot { static constexpr auto name = "not"; }; +struct NameAnd +{ + static constexpr auto name = "and"; +}; +struct NameOr +{ + static constexpr auto name = "or"; +}; +struct NameXor +{ + static constexpr auto name = "xor"; +}; +struct NameNot +{ + static constexpr auto name = "not"; +}; using FunctionAnd = FunctionAnyArityLogical; using FunctionOr = FunctionAnyArityLogical; using FunctionXor = FunctionAnyArityLogical; using FunctionNot = FunctionUnaryLogical; -} +} // namespace DB diff --git a/tests/fullstack-test/expr/logical_op.test b/tests/fullstack-test/expr/logical_op.test index 7d6b8bd2be2..b64b4ff35e3 100644 --- a/tests/fullstack-test/expr/logical_op.test +++ b/tests/fullstack-test/expr/logical_op.test @@ -1,14 +1,19 @@ mysql> drop table if exists test.t1; mysql> drop table if exists test.t2; +mysql> drop table if exists test.t3; mysql> create table test.t1(a char(20),b double); mysql> create table test.t2(a char(20)); +mysql> create table test.t3(a int); mysql> insert into test.t1 values(1,null),('j',0),(1,12.991),(0,0),(0,0),('they',1.009),('can',-99),(0,12.991),(1,-9.183),(null,1); mysql> insert into test.t2 values(0),(0),(0),(0),(0),(0),(1),('with'),('see'),(null); +mysql> insert into test.t3 values(0),(1); mysql> alter table test.t1 set tiflash replica 1; mysql> alter table test.t2 set tiflash replica 1; +mysql> alter table test.t3 set tiflash replica 1; func> wait_table test t1 func> wait_table test t2 +func> wait_table test t3 mysql> set session tidb_isolation_read_engines='tiflash'; select count(*) from test.t1 where (b between null and 100) is null; +----------+ @@ -24,5 +29,44 @@ mysql> set session tidb_allow_mpp=1; set session tidb_isolation_read_engines='ti | 10 | +----------+ +mysql> set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select null and a > 0, a from test.t3; ++----------------+------+ +| null and a > 0 | a | ++----------------+------+ +| 0 | 0 | +| NULL | 1 | ++----------------+------+ + +mysql> set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select null or a > 0, a from test.t3; ++---------------+------+ +| null or a > 0 | a | ++---------------+------+ +| NULL | 0 | +| 1 | 1 | ++---------------+------+ + +mysql> set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select null xor a > 0, a from test.t3; ++----------------+------+ +| null xor a > 0 | a | ++----------------+------+ +| NULL | 0 | +| NULL | 1 | ++----------------+------+ + +mysql> set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select !null, a from test.t3; ++-------+------+ +| !null | a | ++-------+------+ +| NULL | 0 | +| NULL | 1 | ++-------+------+ + +mysql> set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select count(*) from test.t3 group by a having min(null) and a > 0; +# empty + +mysql> set tidb_enforce_mpp=1; set tidb_isolation_read_engines='tiflash'; select count(*) from test.t3 group by a having ifnull(null,count(*)) and min(null); +# empty + #mysql> drop table if exists test.t1; #mysql> drop table if exists test.t2; +#mysql> drop table if exists test.t3;