From 973cd5e972192a2139c20191bf26c75c8a03183f Mon Sep 17 00:00:00 2001
From: Amos Bird <amosbird@gmail.com>
Date: Thu, 21 Sep 2023 22:46:16 +0800
Subject: [PATCH] Always allow nullable keys for projections

---
 src/Storages/MergeTree/MergeTreeData.cpp           |  9 +++++----
 src/Storages/MergeTree/MergeTreeData.h             | 14 ++++++++++++--
 .../01710_projection_with_nullable_keys.reference  |  1 +
 .../01710_projection_with_nullable_keys.sql        |  9 +++++++++
 4 files changed, 27 insertions(+), 6 deletions(-)
 create mode 100644 tests/queries/0_stateless/01710_projection_with_nullable_keys.reference
 create mode 100644 tests/queries/0_stateless/01710_projection_with_nullable_keys.sql

diff --git a/src/Storages/MergeTree/MergeTreeData.cpp b/src/Storages/MergeTree/MergeTreeData.cpp
index 14c9961f6c3..1612eca217c 100644
--- a/src/Storages/MergeTree/MergeTreeData.cpp
+++ b/src/Storages/MergeTree/MergeTreeData.cpp
@@ -481,6 +481,7 @@ void MergeTreeData::checkProperties(
     const StorageInMemoryMetadata & old_metadata,
     bool attach,
     bool allow_empty_sorting_key,
+    bool allow_nullable_key_,
     ContextPtr local_context) const
 {
     if (!new_metadata.sorting_key.definition_ast && !allow_empty_sorting_key)
@@ -598,12 +599,12 @@ void MergeTreeData::checkProperties(
 
             /// We cannot alter a projection so far. So here we do not try to find a projection in old metadata.
             bool is_aggregate = projection.type == ProjectionDescription::Type::Aggregate;
-            checkProperties(*projection.metadata, *projection.metadata, attach, is_aggregate, local_context);
+            checkProperties(*projection.metadata, *projection.metadata, attach, is_aggregate, true /* allow_nullable_key */, local_context);
             projections_names.insert(projection.name);
         }
     }
 
-    checkKeyExpression(*new_sorting_key.expression, new_sorting_key.sample_block, "Sorting", allow_nullable_key);
+    checkKeyExpression(*new_sorting_key.expression, new_sorting_key.sample_block, "Sorting", allow_nullable_key_);
 }
 
 void MergeTreeData::setProperties(
@@ -612,7 +613,7 @@ void MergeTreeData::setProperties(
     bool attach,
     ContextPtr local_context)
 {
-    checkProperties(new_metadata, old_metadata, attach, false, local_context);
+    checkProperties(new_metadata, old_metadata, attach, false, allow_nullable_key, local_context);
     setInMemoryMetadata(new_metadata);
 }
 
@@ -3350,7 +3351,7 @@ void MergeTreeData::checkAlterIsPossible(const AlterCommands & commands, Context
         }
     }
 
-    checkProperties(new_metadata, old_metadata, false, false, local_context);
+    checkProperties(new_metadata, old_metadata, false, false, allow_nullable_key, local_context);
     checkTTLExpressions(new_metadata, old_metadata);
 
     if (!columns_to_check_conversion.empty())
diff --git a/src/Storages/MergeTree/MergeTreeData.h b/src/Storages/MergeTree/MergeTreeData.h
index 6f9779bde00..54b13b0b93d 100644
--- a/src/Storages/MergeTree/MergeTreeData.h
+++ b/src/Storages/MergeTree/MergeTreeData.h
@@ -1246,9 +1246,19 @@ protected:
     /// The same for clearOldTemporaryDirectories.
     std::mutex clear_old_temporary_directories_mutex;
 
-    void checkProperties(const StorageInMemoryMetadata & new_metadata, const StorageInMemoryMetadata & old_metadata, bool attach, bool allow_empty_sorting_key, ContextPtr local_context) const;
+    void checkProperties(
+        const StorageInMemoryMetadata & new_metadata,
+        const StorageInMemoryMetadata & old_metadata,
+        bool attach,
+        bool allow_empty_sorting_key,
+        bool allow_nullable_key_,
+        ContextPtr local_context) const;
 
-    void setProperties(const StorageInMemoryMetadata & new_metadata, const StorageInMemoryMetadata & old_metadata, bool attach = false, ContextPtr local_context = nullptr);
+    void setProperties(
+        const StorageInMemoryMetadata & new_metadata,
+        const StorageInMemoryMetadata & old_metadata,
+        bool attach = false,
+        ContextPtr local_context = nullptr);
 
     void checkPartitionKeyAndInitMinMax(const KeyDescription & new_partition_key);
 
diff --git a/tests/queries/0_stateless/01710_projection_with_nullable_keys.reference b/tests/queries/0_stateless/01710_projection_with_nullable_keys.reference
new file mode 100644
index 00000000000..d1d9e99b4ed
--- /dev/null
+++ b/tests/queries/0_stateless/01710_projection_with_nullable_keys.reference
@@ -0,0 +1 @@
+CREATE TABLE default.sales\n(\n    `DATE_SOLD` DateTime64(3, \'UTC\'),\n    `PRODUCT_ID` Nullable(String),\n    PROJECTION test\n    (\n        SELECT toInt64(count(*))\n        GROUP BY \n            PRODUCT_ID,\n            DATE_SOLD\n    )\n)\nENGINE = MergeTree\nPARTITION BY toYYYYMM(DATE_SOLD)\nORDER BY DATE_SOLD\nSETTINGS index_granularity = 8192
diff --git a/tests/queries/0_stateless/01710_projection_with_nullable_keys.sql b/tests/queries/0_stateless/01710_projection_with_nullable_keys.sql
new file mode 100644
index 00000000000..72757a1d789
--- /dev/null
+++ b/tests/queries/0_stateless/01710_projection_with_nullable_keys.sql
@@ -0,0 +1,9 @@
+DROP TABLE IF EXISTS sales;
+
+CREATE TABLE sales (DATE_SOLD DateTime64(3, 'UTC'), PRODUCT_ID Nullable(String)) Engine MergeTree() PARTITION BY toYYYYMM(DATE_SOLD) ORDER BY DATE_SOLD;
+
+ALTER TABLE sales ADD PROJECTION test (SELECT toInt64(COUNT(*)) GROUP BY PRODUCT_ID, DATE_SOLD);
+
+SHOW CREATE sales;
+
+DROP TABLE sales;