diff --git a/dbms/src/Columns/ColumnConst.cpp b/dbms/src/Columns/ColumnConst.cpp index fa281473200..3703d24f1cb 100644 --- a/dbms/src/Columns/ColumnConst.cpp +++ b/dbms/src/Columns/ColumnConst.cpp @@ -102,9 +102,4 @@ void ColumnConst::getPermutation(bool /*reverse*/, size_t /*limit*/, int /*nan_d res[i] = i; } -bool isColumnConst(const IColumn & column) -{ - return checkColumn(column); -} - } diff --git a/dbms/src/Columns/ColumnConst.h b/dbms/src/Columns/ColumnConst.h index 91cf1748522..2e610b1e335 100644 --- a/dbms/src/Columns/ColumnConst.h +++ b/dbms/src/Columns/ColumnConst.h @@ -197,6 +197,7 @@ public: return false; } + bool isNullable() const override { return isColumnNullable(*data); } bool onlyNull() const override { return data->isNullAt(0); } bool isNumeric() const override { return data->isNumeric(); } bool isFixedAndContiguous() const override { return data->isFixedAndContiguous(); } @@ -216,6 +217,4 @@ public: T getValue() const { return getField().safeGet>(); } }; -bool isColumnConst(const IColumn & column); - } diff --git a/dbms/src/Columns/ColumnLowCardinality.h b/dbms/src/Columns/ColumnLowCardinality.h index d36b91b0c40..afdc397d461 100644 --- a/dbms/src/Columns/ColumnLowCardinality.h +++ b/dbms/src/Columns/ColumnLowCardinality.h @@ -146,6 +146,7 @@ public: size_t sizeOfValueIfFixed() const override { return getDictionary().sizeOfValueIfFixed(); } bool isNumeric() const override { return getDictionary().isNumeric(); } bool lowCardinality() const override { return true; } + bool isNullable() const override { return isColumnNullable(*dictionary.getColumnUniquePtr()); } const IColumnUnique & getDictionary() const { return dictionary.getColumnUnique(); } const ColumnPtr & getDictionaryPtr() const { return dictionary.getColumnUniquePtr(); } diff --git a/dbms/src/Columns/ColumnNullable.cpp b/dbms/src/Columns/ColumnNullable.cpp index 7ef6b5e85c9..3ae692552e8 100644 --- a/dbms/src/Columns/ColumnNullable.cpp +++ b/dbms/src/Columns/ColumnNullable.cpp @@ -451,10 +451,9 @@ void ColumnNullable::checkConsistency() const ErrorCodes::SIZES_OF_NESTED_COLUMNS_ARE_INCONSISTENT); } - ColumnPtr makeNullable(const ColumnPtr & column) { - if (checkColumn(*column)) + if (isColumnNullable(*column)) return column; if (isColumnConst(*column)) @@ -463,11 +462,4 @@ ColumnPtr makeNullable(const ColumnPtr & column) return ColumnNullable::create(column, ColumnUInt8::create(column->size(), 0)); } -bool isNullable(const IColumn & column) -{ - if (auto * column_const = checkAndGetColumn(column)) - return checkColumn(column_const->getDataColumn()); - return checkColumn(column); -} - } diff --git a/dbms/src/Columns/ColumnNullable.h b/dbms/src/Columns/ColumnNullable.h index a82be4d55e7..b4db692b61d 100644 --- a/dbms/src/Columns/ColumnNullable.h +++ b/dbms/src/Columns/ColumnNullable.h @@ -100,6 +100,7 @@ public: return false; } + bool isNullable() const override { return true; } bool isFixedAndContiguous() const override { return false; } bool valuesHaveFixedSize() const override { return nested_column->valuesHaveFixedSize(); } size_t sizeOfValueIfFixed() const override { return null_map->sizeOfValueIfFixed() + nested_column->sizeOfValueIfFixed(); } @@ -141,8 +142,6 @@ private: void applyNullMapImpl(const ColumnUInt8 & map); }; - ColumnPtr makeNullable(const ColumnPtr & column); -bool isNullable(const IColumn & column); } diff --git a/dbms/src/Columns/ColumnUnique.h b/dbms/src/Columns/ColumnUnique.h index 6cbaf807f53..715cb966018 100644 --- a/dbms/src/Columns/ColumnUnique.h +++ b/dbms/src/Columns/ColumnUnique.h @@ -191,7 +191,7 @@ ColumnUnique::ColumnUnique(MutableColumnPtr && holder, bool is_nulla { if (column_holder->size() < numSpecialValues()) throw Exception("Too small holder column for ColumnUnique.", ErrorCodes::ILLEGAL_COLUMN); - if (checkColumn(*column_holder)) + if (isColumnNullable(*column_holder)) throw Exception("Holder column for ColumnUnique can't be nullable.", ErrorCodes::ILLEGAL_COLUMN); index.setColumn(getRawColumnPtr()); diff --git a/dbms/src/Columns/IColumn.cpp b/dbms/src/Columns/IColumn.cpp index 43b0b25f375..cb6f7e9edf3 100644 --- a/dbms/src/Columns/IColumn.cpp +++ b/dbms/src/Columns/IColumn.cpp @@ -1,6 +1,8 @@ #include #include #include +#include +#include namespace DB @@ -22,4 +24,14 @@ String IColumn::dumpStructure() const return res.str(); } +bool isColumnNullable(const IColumn & column) +{ + return checkColumn(column); +} + +bool isColumnConst(const IColumn & column) +{ + return checkColumn(column); +} + } diff --git a/dbms/src/Columns/IColumn.h b/dbms/src/Columns/IColumn.h index c7a71e97286..3955ed4b528 100644 --- a/dbms/src/Columns/IColumn.h +++ b/dbms/src/Columns/IColumn.h @@ -297,6 +297,9 @@ public: /// Various properties on behaviour of column type. + /// True if column contains something nullable inside. It's true for ColumnNullable, can be true or false for ColumnConst, etc. + virtual bool isNullable() const { return false; } + /// It's a special kind of column, that contain single value, but is not a ColumnConst. virtual bool isDummy() const { return false; } @@ -429,6 +432,10 @@ bool checkColumn(const IColumn * column) return checkAndGetColumn(column); } -bool isColumnConst(const IColumn & column); /// defined in ColumnConst.cpp +/// True if column's an ColumnConst instance. It's just a syntax sugar for type check. +bool isColumnConst(const IColumn & column); + +/// True if column's an ColumnNullable instance. It's just a syntax sugar for type check. +bool isColumnNullable(const IColumn & column); } diff --git a/dbms/src/Functions/array/arrayElement.cpp b/dbms/src/Functions/array/arrayElement.cpp index 704bf82c3a9..c9293da9433 100644 --- a/dbms/src/Functions/array/arrayElement.cpp +++ b/dbms/src/Functions/array/arrayElement.cpp @@ -749,12 +749,12 @@ void FunctionArrayElement::executeImpl(Block & block, const ColumnNumbers & argu col_array = checkAndGetColumn(block.getByPosition(arguments[0]).column.get()); if (col_array) - is_array_of_nullable = checkColumn(col_array->getData()); + is_array_of_nullable = isColumnNullable(col_array->getData()); else { col_const_array = checkAndGetColumnConstData(block.getByPosition(arguments[0]).column.get()); if (col_const_array) - is_array_of_nullable = checkColumn(col_const_array->getData()); + is_array_of_nullable = isColumnNullable(col_const_array->getData()); else throw Exception("Illegal column " + block.getByPosition(arguments[0]).column->getName() + " of first argument of function " + getName(), ErrorCodes::ILLEGAL_COLUMN); diff --git a/dbms/src/Functions/coalesce.cpp b/dbms/src/Functions/coalesce.cpp index 4ba0bb34f80..7ef7eeadfdb 100644 --- a/dbms/src/Functions/coalesce.cpp +++ b/dbms/src/Functions/coalesce.cpp @@ -7,6 +7,7 @@ #include #include #include +#include namespace DB @@ -150,11 +151,14 @@ public: ColumnPtr res = std::move(temp_block.getByPosition(result).column); /// if last argument is not nullable, result should be also not nullable - if (!isNullable(*block.getByPosition(multi_if_args.back()).column) && isNullable(*res)) + if (!block.getByPosition(multi_if_args.back()).column->isNullable() && res->isNullable()) { - if (auto * column_const = checkAndGetColumn(*res)) + if (auto * column_lc = checkAndGetColumn(*res)) + res = checkAndGetColumn(*column_lc->convertToFullColumn())->getNestedColumnPtr(); + else if (auto * column_const = checkAndGetColumn(*res)) res = checkAndGetColumn(column_const->getDataColumn())->getNestedColumnPtr(); - res = checkAndGetColumn(*res)->getNestedColumnPtr(); + else + res = checkAndGetColumn(*res)->getNestedColumnPtr(); } block.getByPosition(result).column = std::move(res); diff --git a/dbms/src/Functions/if.cpp b/dbms/src/Functions/if.cpp index 81485fc2eb0..6676ad87d75 100644 --- a/dbms/src/Functions/if.cpp +++ b/dbms/src/Functions/if.cpp @@ -693,7 +693,7 @@ private: static ColumnPtr makeNullableColumnIfNot(const ColumnPtr & column) { - if (checkColumn(*column)) + if (isColumnNullable(*column)) return column; return ColumnNullable::create( @@ -813,7 +813,7 @@ private: { if (cond_col) { - if (checkColumn(*arg_else.column)) + if (isColumnNullable(*arg_else.column)) { auto arg_else_column = arg_else.column; auto result_column = (*std::move(arg_else_column)).mutate(); @@ -855,7 +855,7 @@ private: for (size_t i = 0; i < size; ++i) negated_null_map_data[i] = !null_map_data[i]; - if (checkColumn(*arg_then.column)) + if (isColumnNullable(*arg_then.column)) { auto arg_then_column = arg_then.column; auto result_column = (*std::move(arg_then_column)).mutate(); diff --git a/dbms/src/Functions/multiIf.cpp b/dbms/src/Functions/multiIf.cpp index fce2114947c..7ee5f1c5e67 100644 --- a/dbms/src/Functions/multiIf.cpp +++ b/dbms/src/Functions/multiIf.cpp @@ -152,7 +152,7 @@ public: } else { - if (checkColumn(*cond_col.column)) + if (isColumnNullable(*cond_col.column)) instruction.condition_is_nullable = true; instruction.condition = cond_col.column.get(); diff --git a/dbms/src/Interpreters/Aggregator.h b/dbms/src/Interpreters/Aggregator.h index 1e178066374..bbf390f80aa 100644 --- a/dbms/src/Interpreters/Aggregator.h +++ b/dbms/src/Interpreters/Aggregator.h @@ -308,7 +308,7 @@ struct AggregationMethodKeysFixed ColumnUInt8 * null_map; /// If we have a nullable column, get its nested column and its null map. - if (has_nullable_keys && checkColumn(*key_columns[i])) + if (has_nullable_keys && isColumnNullable(*key_columns[i])) { ColumnNullable & nullable_col = static_cast(*key_columns[i]); observed_column = &nullable_col.getNestedColumn(); @@ -321,7 +321,7 @@ struct AggregationMethodKeysFixed } bool is_null; - if (has_nullable_keys && checkColumn(*key_columns[i])) + if (has_nullable_keys && isColumnNullable(*key_columns[i])) { /// The current column is nullable. Check if the value of the /// corresponding key is nullable. Update the null map accordingly. diff --git a/dbms/src/Interpreters/Join.cpp b/dbms/src/Interpreters/Join.cpp index 877b1a4fd64..a7e5074650b 100644 --- a/dbms/src/Interpreters/Join.cpp +++ b/dbms/src/Interpreters/Join.cpp @@ -1411,7 +1411,7 @@ private: const auto & dst = out_block.getByPosition(key_pos).column; const auto & src = sample_block_with_keys.getByPosition(i).column; - if (checkColumn(*dst) != checkColumn(*src)) + if (isColumnNullable(*dst) != isColumnNullable(*src)) nullability_changes.insert(key_pos); } diff --git a/dbms/tests/queries/0_stateless/00957_coalesce_const_nullable_crash.reference b/dbms/tests/queries/0_stateless/00957_coalesce_const_nullable_crash.reference index 437b07c062c..61841cdb8f1 100644 --- a/dbms/tests/queries/0_stateless/00957_coalesce_const_nullable_crash.reference +++ b/dbms/tests/queries/0_stateless/00957_coalesce_const_nullable_crash.reference @@ -2,3 +2,10 @@ 2 Nullable(UInt8) 1 Nullable(UInt8) \N Nullable(Nothing) +1 Nullable(UInt8) +2 Nullable(UInt8) +1 Nullable(UInt8) +\N Nullable(Nothing) +1 LowCardinality(Nullable(UInt8)) +2 LowCardinality(Nullable(UInt8)) +1 LowCardinality(Nullable(UInt8)) diff --git a/dbms/tests/queries/0_stateless/00957_coalesce_const_nullable_crash.sql b/dbms/tests/queries/0_stateless/00957_coalesce_const_nullable_crash.sql index e13b69e6f7a..39e148c5751 100644 --- a/dbms/tests/queries/0_stateless/00957_coalesce_const_nullable_crash.sql +++ b/dbms/tests/queries/0_stateless/00957_coalesce_const_nullable_crash.sql @@ -2,3 +2,12 @@ SELECT coalesce(toNullable(1), toNullable(2)) as x, toTypeName(x); SELECT coalesce(NULL, toNullable(2)) as x, toTypeName(x); SELECT coalesce(toNullable(1), NULL) as x, toTypeName(x); SELECT coalesce(NULL, NULL) as x, toTypeName(x); + +SELECT coalesce(toNullable(materialize(1)), toNullable(materialize(2))) as x, toTypeName(x); +SELECT coalesce(NULL, toNullable(materialize(2))) as x, toTypeName(x); +SELECT coalesce(toNullable(materialize(1)), NULL) as x, toTypeName(x); +SELECT coalesce(materialize(NULL), materialize(NULL)) as x, toTypeName(x); + +SELECT coalesce(toLowCardinality(toNullable(1)), toLowCardinality(toNullable(2))) as x, toTypeName(x); +SELECT coalesce(NULL, toLowCardinality(toNullable(2))) as x, toTypeName(x); +SELECT coalesce(toLowCardinality(toNullable(1)), NULL) as x, toTypeName(x);