Unifying column names validation for both row and column tables #14722

ydb/core/tx/schemeshard/olap/columns/update.cpp

@@ -1,5 +1,6 @@
 #include "update.h"
 #include <ydb/core/tx/schemeshard/schemeshard_info_types.h>
+#include <ydb/core/tx/schemeshard/schemeshard_utils.h>
 #include <yql/essentials/minikql/mkql_type_ops.h>
 #include <ydb/core/scheme/scheme_types_proto.h>
 #include <ydb/core/scheme_types/scheme_type_registry.h>
@@ -53,6 +54,10 @@ bool TOlapColumnBase::ParseFromRequest(const NKikimrSchemeOp::TOlapColumnDescrip
         return false;
     }
     Name = columnSchema.GetName();
+    if (!IsValidColumnName(Name, false)) {
+        errors.AddError(Sprintf("Invalid name for column '%s'", Name.data()));
+        return false;
+    }
     NotNullFlag = columnSchema.GetNotNull();
     TypeName = columnSchema.GetType();
     StorageId = columnSchema.GetStorageId();

ydb/core/tx/schemeshard/ut_olap/ut_olap.cpp

@@ -30,6 +30,20 @@ static const TString defaultStoreSchema = R"(
     }
 )";
 
+static const TString invalidStoreSchema = R"(
+    Name: "OlapStore"
+    ColumnShardCount: 1
+    SchemaPresets {
+        Name: "default"
+        Schema {
+            Columns { Name: "timestamp" Type: "Timestamp" NotNull: true }
+            Columns { Name: "data" Type: "Utf8" }
+            Columns { Name: "mess age" Type: "Utf8" }
+            KeyColumnNames: "timestamp"
+        }
+    }
+)";
+
 static const TString defaultTableSchema = R"(
     Name: "ColumnTable"
     ColumnShardCount: 1
@@ -45,7 +59,21 @@ static const TVector<NArrow::NTest::TTestColumn> defaultYdbSchema = {
     NArrow::NTest::TTestColumn("data", TTypeInfo(NTypeIds::Utf8) )
 };
 
-}}
+static const TString tableSchemaFormat = R"(
+    Name: "TestTable"
+    Schema {
+        Columns {
+            Name: "Id"
+            Type: "Int32"
+            NotNull: True
+        }
+        Columns {
+            Name: "%s"
+            Type: "Utf8"
+        }
+        KeyColumnNames: ["Id"]
+    }
+)";
 
 #define DEBUG_HINT (TStringBuilder() << "at line " << __LINE__)
 
@@ -99,6 +127,7 @@ NKikimrTxDataShard::TEvPeriodicTableStats WaitTableStats(TTestActorRuntime& runt
 
     return stats;
 }
+}}
 
 Y_UNIT_TEST_SUITE(TOlap) {
     Y_UNIT_TEST(CreateStore) {
@@ -1136,3 +1165,162 @@ Y_UNIT_TEST_SUITE(TOlap) {
         CheckQuotaExceedance(runtime, TTestTxConfig::SchemeShard, "/MyRoot/SomeDatabase", false, DEBUG_HINT);
     }
 }
+
+Y_UNIT_TEST_SUITE(TOlapNaming) {
+
+    Y_UNIT_TEST(CreateColumnTableOk) {
+        TTestBasicRuntime runtime;
+        TTestEnv env(runtime);
+        ui64 txId = 100;
+
+        TString allowedChars = "_-0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
+
+        TString tableSchema = Sprintf(tableSchemaFormat.c_str(), allowedChars.c_str());
+
+        TestCreateColumnTable(runtime, ++txId, "/MyRoot", tableSchema, {NKikimrScheme::StatusAccepted});
+        env.TestWaitNotification(runtime, txId);
+    }
+
+    Y_UNIT_TEST(CreateColumnTableFailed) {
+        TTestBasicRuntime runtime;
+        TTestEnv env(runtime);
+        ui64 txId = 100;
+
+        TVector<TString> notAllowedNames = {"mess age", "~!@#$%^&*()+=asdfa"};
+
+        for (const auto& colName: notAllowedNames) {
+            TString tableSchema = Sprintf(tableSchemaFormat.c_str(), colName.c_str());
+
+            TestCreateColumnTable(runtime, ++txId, "/MyRoot", tableSchema, {NKikimrScheme::StatusSchemeError});
+            env.TestWaitNotification(runtime, txId);
+        }
+    }
+
+    Y_UNIT_TEST(CreateColumnStoreOk) {
+        TTestBasicRuntime runtime;
+        TTestEnv env(runtime);
+        ui64 txId = 100;
+
+        const TString& storeSchema = defaultStoreSchema;
+
+        TestCreateOlapStore(runtime, ++txId, "/MyRoot", storeSchema, {NKikimrScheme::StatusAccepted});
+        env.TestWaitNotification(runtime, txId);
+
+        TestLs(runtime, "/MyRoot/OlapStore", false, NLs::PathExist);
+    }
+
+    Y_UNIT_TEST(CreateColumnStoreFailed) {
+        TTestBasicRuntime runtime;
+        TTestEnv env(runtime);
+        ui64 txId = 100;
+
+        const TString& storeSchema = invalidStoreSchema;
+
+        TestCreateOlapStore(runtime, ++txId, "/MyRoot", storeSchema, {NKikimrScheme::StatusSchemeError});
+        env.TestWaitNotification(runtime, txId);
+
+        TestLs(runtime, "/MyRoot/OlapStore", false, NLs::PathNotExist);
+    }
+
+    Y_UNIT_TEST(AlterColumnTableOk) {
+        TTestBasicRuntime runtime;
+        TTestEnvOptions options;
+        TTestEnv env(runtime, options);
+        ui64 txId = 100;
+
+        TString tableSchema = Sprintf(tableSchemaFormat.c_str(), "message");
+
+        TestCreateColumnTable(runtime, ++txId, "/MyRoot", tableSchema, {NKikimrScheme::StatusAccepted});
+        env.TestWaitNotification(runtime, txId);
+
+        TestAlterColumnTable(runtime, ++txId, "/MyRoot", R"(
+            Name: "TestTable"
+            AlterSchema {
+                AddColumns {
+                    Name: "NewColumn"
+                    Type: "Int32"
+                }
+            }
+        )");
+        env.TestWaitNotification(runtime, txId);
+    }
+
+    Y_UNIT_TEST(AlterColumnTableFailed) {
+        TTestBasicRuntime runtime;
+        TTestEnvOptions options;
+        TTestEnv env(runtime, options);
+        ui64 txId = 100;
+
+        TString tableSchema = Sprintf(tableSchemaFormat.c_str(), "message");
+
+        TestCreateColumnTable(runtime, ++txId, "/MyRoot", tableSchema, {NKikimrScheme::StatusAccepted});
+        env.TestWaitNotification(runtime, txId);
+
+        TestAlterColumnTable(runtime, ++txId, "/MyRoot", R"(
+            Name: "TestTable"
+            AlterSchema {
+                AddColumns {
+                    Name: "New Column"
+                    Type: "Int32"
+                }
+            }
+        )", {NKikimrScheme::StatusSchemeError});
+        env.TestWaitNotification(runtime, txId);
+    }
+
+    Y_UNIT_TEST(AlterColumnStoreOk) {
+        TTestBasicRuntime runtime;
+        TTestEnv env(runtime);
+        ui64 txId = 100;
+
+        const TString& olapSchema = defaultStoreSchema;
+
+        TestCreateOlapStore(runtime, ++txId, "/MyRoot", olapSchema);
+        env.TestWaitNotification(runtime, txId);
+
+        const TString& tableSchema = defaultTableSchema;
+
+        TestCreateColumnTable(runtime, ++txId, "/MyRoot/OlapStore", tableSchema);
+        env.TestWaitNotification(runtime, txId);
+
+        TestAlterOlapStore(runtime, ++txId, "/MyRoot", R"(
+            Name: "OlapStore"
+            AlterSchemaPresets {
+                Name: "default"
+                AlterSchema {
+                    AddColumns { Name: "comment" Type: "Utf8" }
+                }
+            }
+        )", {NKikimrScheme::StatusAccepted});
+
+        env.TestWaitNotification(runtime, txId);
+    }
+
+    Y_UNIT_TEST(AlterColumnStoreFailed) {
+        TTestBasicRuntime runtime;
+        TTestEnv env(runtime);
+        ui64 txId = 100;
+
+        const TString& olapSchema = defaultStoreSchema;
+
+        TestCreateOlapStore(runtime, ++txId, "/MyRoot", olapSchema);
+        env.TestWaitNotification(runtime, txId);
+
+        const TString& tableSchema = defaultTableSchema;
+
+        TestCreateColumnTable(runtime, ++txId, "/MyRoot/OlapStore", tableSchema);
+        env.TestWaitNotification(runtime, txId);
+
+        TestAlterOlapStore(runtime, ++txId, "/MyRoot", R"(
+            Name: "OlapStore"
+            AlterSchemaPresets {
+                Name: "default"
+                AlterSchema {
+                    AddColumns { Name: "mess age" Type: "Utf8" }
+                }
+            }
+        )", {NKikimrScheme::StatusSchemeError});
+
+        env.TestWaitNotification(runtime, txId);
+    }
+}