Merge pull request #23237 from vdimir/fix-join-lowcard-nullable

src/Columns/ColumnLowCardinality.h

@@ -187,6 +187,7 @@ public:
      * So LC(Nullable(T)) would return true, LC(U) -- false.
      */
     bool nestedIsNullable() const { return isColumnNullable(*dictionary.getColumnUnique().getNestedColumn()); }
+    void nestedToNullable() { dictionary.getColumnUnique().nestedToNullable(); }
 
     const IColumnUnique & getDictionary() const { return dictionary.getColumnUnique(); }
     const ColumnPtr & getDictionaryPtr() const { return dictionary.getColumnUniquePtr(); }

src/Columns/ColumnUnique.h

@@ -50,6 +50,7 @@ public:
     const ColumnPtr & getNestedColumn() const override;
     const ColumnPtr & getNestedNotNullableColumn() const override { return column_holder; }
     bool nestedColumnIsNullable() const override { return is_nullable; }
+    void nestedToNullable() override;
 
     size_t uniqueInsert(const Field & x) override;
     size_t uniqueInsertFrom(const IColumn & src, size_t n) override;
@@ -263,6 +264,13 @@ void ColumnUnique<ColumnType>::updateNullMask()
     }
 }
 
+template <typename ColumnType>
+void ColumnUnique<ColumnType>::nestedToNullable()
+{
+    is_nullable = true;
+    createNullMask();
+}
+
 template <typename ColumnType>
 const ColumnPtr & ColumnUnique<ColumnType>::getNestedColumn() const
 {

src/Columns/IColumnUnique.h

@@ -24,6 +24,7 @@ public:
     virtual const ColumnPtr & getNestedNotNullableColumn() const = 0;
 
     virtual bool nestedColumnIsNullable() const = 0;
+    virtual void nestedToNullable() = 0;
 
     /// Returns array with StringRefHash calculated for each row of getNestedNotNullableColumn() column.
     /// Returns nullptr if nested column doesn't contain strings. Otherwise calculates hash (if it wasn't).

src/Interpreters/TableJoin.cpp

@@ -214,12 +214,12 @@ Block TableJoin::getRequiredRightKeys(const Block & right_table_keys, std::vecto
 
 bool TableJoin::leftBecomeNullable(const DataTypePtr & column_type) const
 {
-    return forceNullableLeft() && column_type->canBeInsideNullable();
+    return forceNullableLeft() && JoinCommon::canBecomeNullable(column_type);
 }
 
 bool TableJoin::rightBecomeNullable(const DataTypePtr & column_type) const
 {
-    return forceNullableRight() && column_type->canBeInsideNullable();
+    return forceNullableRight() && JoinCommon::canBecomeNullable(column_type);
 }
 
 void TableJoin::addJoinedColumn(const NameAndTypePair & joined_column)
@@ -233,7 +233,7 @@ void TableJoin::addJoinedColumn(const NameAndTypePair & joined_column)
     }
 
     if (rightBecomeNullable(type))
-        type = makeNullable(type);
+        type = JoinCommon::convertTypeToNullable(type);
 
     columns_added_by_join.emplace_back(joined_column.name, type);
 }
@@ -265,7 +265,7 @@ void TableJoin::addJoinedColumnsAndCorrectTypes(ColumnsWithTypeAndName & columns
             /// No need to nullify constants
             bool is_column_const = col.column && isColumnConst(*col.column);
             if (!is_column_const)
-                col.type = makeNullable(col.type);
+                col.type = JoinCommon::convertTypeToNullable(col.type);
         }
     }
 

src/Interpreters/join_common.cpp

@@ -49,20 +49,53 @@ ColumnPtr changeLowCardinality(const ColumnPtr & column, const ColumnPtr & dst_s
 namespace JoinCommon
 {
 
-void convertColumnToNullable(ColumnWithTypeAndName & column, bool low_card_nullability)
+
+bool canBecomeNullable(const DataTypePtr & type)
 {
-    if (low_card_nullability && column.type->lowCardinality())
+    bool can_be_inside = type->canBeInsideNullable();
+    if (const auto * low_cardinality_type = typeid_cast<const DataTypeLowCardinality *>(type.get()))
+        can_be_inside |= low_cardinality_type->getDictionaryType()->canBeInsideNullable();
+    return can_be_inside;
+}
+
+/// Add nullability to type.
+/// Note: LowCardinality(T) transformed to LowCardinality(Nullable(T))
+DataTypePtr convertTypeToNullable(const DataTypePtr & type)
+{
+    if (const auto * low_cardinality_type = typeid_cast<const DataTypeLowCardinality *>(type.get()))
+    {
+        const auto & dict_type = low_cardinality_type->getDictionaryType();
+        if (dict_type->canBeInsideNullable())
+            return std::make_shared<DataTypeLowCardinality>(makeNullable(dict_type));
+    }
+    return makeNullable(type);
+}
+
+void convertColumnToNullable(ColumnWithTypeAndName & column, bool remove_low_card)
+{
+    if (remove_low_card && column.type->lowCardinality())
     {
         column.column = recursiveRemoveLowCardinality(column.column);
         column.type = recursiveRemoveLowCardinality(column.type);
     }
 
-    if (column.type->isNullable() || !column.type->canBeInsideNullable())
+    if (column.type->isNullable() || !canBecomeNullable(column.type))
         return;
 
-    column.type = makeNullable(column.type);
+    column.type = convertTypeToNullable(column.type);
+
     if (column.column)
-        column.column = makeNullable(column.column);
+    {
+        if (column.column->lowCardinality())
+        {
+            /// Convert nested to nullable, not LowCardinality itself
+            ColumnLowCardinality * col_as_lc = assert_cast<ColumnLowCardinality *>(column.column->assumeMutable().get());
+            if (!col_as_lc->nestedIsNullable())
+                col_as_lc->nestedToNullable();
+        }
+        else
+            column.column = makeNullable(column.column);
+    }
 }
 
 void convertColumnsToNullable(Block & block, size_t starting_pos)

src/Interpreters/join_common.h

@@ -15,8 +15,9 @@ using ColumnRawPtrs = std::vector<const IColumn *>;
 
 namespace JoinCommon
 {
-
-void convertColumnToNullable(ColumnWithTypeAndName & column, bool low_card_nullability = false);
+bool canBecomeNullable(const DataTypePtr & type);
+DataTypePtr convertTypeToNullable(const DataTypePtr & type);
+void convertColumnToNullable(ColumnWithTypeAndName & column, bool remove_low_card = false);
 void convertColumnsToNullable(Block & block, size_t starting_pos = 0);
 void removeColumnNullability(ColumnWithTypeAndName & column);
 void changeColumnRepresentation(const ColumnPtr & src_column, ColumnPtr & dst_column);

tests/queries/0_stateless/01049_join_low_card_bug.reference

@@ -38,22 +38,22 @@ str	Nullable(String)
 \N	str	Nullable(String)	LowCardinality(String)
 \N	str	Nullable(String)	LowCardinality(String)
 \N	str	Nullable(String)	LowCardinality(String)
-	LowCardinality(String)
-str	LowCardinality(String)
-	LowCardinality(String)
-str	LowCardinality(String)
-	str	LowCardinality(String)
-	str	LowCardinality(String)
-	str	LowCardinality(String)
-	str	LowCardinality(String)
-	LowCardinality(String)
-str	LowCardinality(String)
-	LowCardinality(String)
-str	LowCardinality(String)
-	str	LowCardinality(String)
-	str	LowCardinality(String)
-	str	LowCardinality(String)
-	str	LowCardinality(String)
+\N	LowCardinality(Nullable(String))
+str	LowCardinality(Nullable(String))
+\N	LowCardinality(Nullable(String))
+str	LowCardinality(Nullable(String))
+\N	str	LowCardinality(Nullable(String))
+\N	str	LowCardinality(Nullable(String))
+\N	str	LowCardinality(Nullable(String))
+\N	str	LowCardinality(Nullable(String))
+\N	LowCardinality(Nullable(String))
+str	LowCardinality(Nullable(String))
+\N	LowCardinality(Nullable(String))
+str	LowCardinality(Nullable(String))
+\N	str	LowCardinality(Nullable(String))
+\N	str	LowCardinality(Nullable(String))
+\N	str	LowCardinality(Nullable(String))
+\N	str	LowCardinality(Nullable(String))
 \N	Nullable(String)
 str	Nullable(String)
 \N	Nullable(String)
@@ -62,14 +62,14 @@ str	Nullable(String)
 \N	str	Nullable(String)
 \N	str	Nullable(String)
 \N	str	Nullable(String)
-	LowCardinality(String)
-str	LowCardinality(String)
-	LowCardinality(String)
-str	LowCardinality(String)
-	str	LowCardinality(String)
-	str	LowCardinality(String)
-	str	LowCardinality(String)
-	str	LowCardinality(String)
+\N	LowCardinality(Nullable(String))
+str	LowCardinality(Nullable(String))
+\N	LowCardinality(Nullable(String))
+str	LowCardinality(Nullable(String))
+\N	str	LowCardinality(Nullable(String))
+\N	str	LowCardinality(Nullable(String))
+\N	str	LowCardinality(Nullable(String))
+\N	str	LowCardinality(Nullable(String))
 \N	Nullable(String)
 str	Nullable(String)
 \N	Nullable(String)

tests/queries/0_stateless/01049_join_low_card_bug.sql

@@ -76,7 +76,6 @@ SELECT l.lc, r.lc, toTypeName(l.lc), toTypeName(materialize(r.lc)) FROM nl AS l
 SELECT l.lc, r.lc, toTypeName(l.lc), toTypeName(materialize(r.lc)) FROM nl AS l FULL JOIN r_lc AS r USING (x);
 SELECT l.lc, r.lc, toTypeName(l.lc), toTypeName(materialize(r.lc)) FROM nl AS l FULL JOIN r_lc AS r USING (lc);
 
--- TODO: LC nullability
 SET join_use_nulls = 1;
 
 SELECT lc, toTypeName(lc) FROM l_lc AS l RIGHT JOIN r_lc AS r USING (x);

tests/queries/0_stateless/01142_join_lc_and_nullable_in_key.reference

@@ -14,16 +14,16 @@
 1	l	\N	Nullable(String)
 -
 1	l	\N	Nullable(String)
-2		\N	Nullable(String)
+2	\N	\N	Nullable(String)
 1	l	\N	Nullable(String)
-2		\N	Nullable(String)
+2	\N	\N	Nullable(String)
 -
 1	l	\N	Nullable(String)
-\N		\N	Nullable(String)
+\N	\N	\N	Nullable(String)
 1	l	\N	Nullable(String)
-\N		\N	Nullable(String)
+\N	\N	\N	Nullable(String)
 -
 1	l	\N	Nullable(String)
-\N		\N	Nullable(String)
+\N	\N	\N	Nullable(String)
 1	l	\N	Nullable(String)
-\N		\N	Nullable(String)
+\N	\N	\N	Nullable(String)

tests/queries/0_stateless/01142_merge_join_lc_and_nullable_in_key.reference

@@ -14,16 +14,16 @@
 1	l	\N	Nullable(String)
 -
 1	l	\N	Nullable(String)
-2		\N	Nullable(String)
+2	\N	\N	Nullable(String)
 1	l	\N	Nullable(String)
-2		\N	Nullable(String)
+2	\N	\N	Nullable(String)
 -
 1	l	\N	Nullable(String)
-\N		\N	Nullable(String)
+\N	\N	\N	Nullable(String)
 1	l	\N	Nullable(String)
-\N		\N	Nullable(String)
+\N	\N	\N	Nullable(String)
 -
 1	l	\N	Nullable(String)
-\N		\N	Nullable(String)
+\N	\N	\N	Nullable(String)
 1	l	\N	Nullable(String)
-\N		\N	Nullable(String)
+\N	\N	\N	Nullable(String)

tests/queries/0_stateless/01477_lc_in_merge_join_left_key.reference

@@ -19,17 +19,17 @@
 1	\N	l	Nullable(String)	LowCardinality(String)
 -
 1	l	\N	LowCardinality(String)	Nullable(String)
-2		\N	LowCardinality(String)	Nullable(String)
-1	l	\N	LowCardinality(String)	Nullable(String)
-2		\N	LowCardinality(String)	Nullable(String)
+2	\N	\N	LowCardinality(Nullable(String))	Nullable(String)
+1	l	\N	LowCardinality(Nullable(String))	Nullable(String)
+2	\N	\N	LowCardinality(Nullable(String))	Nullable(String)
 -
-\N	\N		Nullable(String)	LowCardinality(String)
+\N	\N	\N	Nullable(String)	LowCardinality(Nullable(String))
 1	\N	l	Nullable(String)	LowCardinality(String)
-1	\N	l	Nullable(String)	LowCardinality(String)
-\N	\N		Nullable(String)	LowCardinality(String)
+1	\N	l	Nullable(String)	LowCardinality(Nullable(String))
+\N	\N	\N	Nullable(String)	LowCardinality(Nullable(String))
 -
 1	l	\N	LowCardinality(String)	Nullable(String)
-\N		\N	LowCardinality(String)	Nullable(String)
-1	l	\N	LowCardinality(String)	Nullable(String)
-\N		\N	LowCardinality(String)	Nullable(String)
+\N	\N	\N	LowCardinality(Nullable(String))	Nullable(String)
+1	l	\N	LowCardinality(Nullable(String))	Nullable(String)
+\N	\N	\N	LowCardinality(Nullable(String))	Nullable(String)
 -