From a348292e839ec47f3faf137b6cc922361f2517a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Poyraz=20K=C3=BC=C3=A7=C3=BCkarslan?= <83272398+PoyrazK@users.noreply.github.com> Date: Sat, 11 Apr 2026 19:30:53 +0300 Subject: [PATCH 1/3] test: add distributed_executor and ShardManager unit tests Add unit tests covering: - ShardManager: stable_hash, compute_shard, get_target_node - DistributedExecutor: DDL succeeds (local catalog), DML/SELECT/transaction fails without nodes - Sharding key extraction via parser (where clause tests) - Null safety: verify correct success/failure per statement type Part of ongoing coverage improvement effort. --- CMakeLists.txt | 1 + tests/distributed_executor_tests.cpp | 341 +++++++++++++++++++++++++++ 2 files changed, 342 insertions(+) create mode 100644 tests/distributed_executor_tests.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 3805406e..5e2087bd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -138,6 +138,7 @@ if(BUILD_TESTS) add_cloudsql_test(rpc_server_tests tests/rpc_server_tests.cpp) add_cloudsql_test(operator_tests tests/operator_tests.cpp) add_cloudsql_test(query_executor_tests tests/query_executor_tests.cpp) + add_cloudsql_test(distributed_executor_tests tests/distributed_executor_tests.cpp) add_custom_target(run-tests COMMAND ${CMAKE_CTEST_COMMAND} diff --git a/tests/distributed_executor_tests.cpp b/tests/distributed_executor_tests.cpp new file mode 100644 index 00000000..7b6372f8 --- /dev/null +++ b/tests/distributed_executor_tests.cpp @@ -0,0 +1,341 @@ +/** + * @file distributed_executor_tests.cpp + * @brief Unit tests for DistributedExecutor and ShardManager utilities + */ + +#include + +#include +#include +#include + +#include "catalog/catalog.hpp" +#include "common/cluster_manager.hpp" +#include "common/config.hpp" +#include "common/value.hpp" +#include "distributed/distributed_executor.hpp" +#include "distributed/shard_manager.hpp" +#include "parser/expression.hpp" +#include "parser/lexer.hpp" +#include "parser/parser.hpp" + +using namespace cloudsql; +using namespace cloudsql::executor; +using namespace cloudsql::cluster; +using namespace cloudsql::parser; +using namespace cloudsql::common; + +namespace { + +// ============= ShardManager Tests ============= + +TEST(ShardManagerTests, StableHashConsistency) { + // Same string should always produce same hash + uint32_t h1 = ShardManager::stable_hash("test_key"); + uint32_t h2 = ShardManager::stable_hash("test_key"); + uint32_t h3 = ShardManager::stable_hash("test_key"); + EXPECT_EQ(h1, h2); + EXPECT_EQ(h2, h3); +} + +TEST(ShardManagerTests, StableHashDifferentStrings) { + // Different strings should likely produce different hashes + uint32_t h1 = ShardManager::stable_hash("key1"); + uint32_t h2 = ShardManager::stable_hash("key2"); + EXPECT_NE(h1, h2); +} + +TEST(ShardManagerTests, StableHashEmptyString) { + uint32_t hash = ShardManager::stable_hash(""); + // Empty string should have a defined hash value (DJB2 algorithm) + EXPECT_EQ(hash, 5381u); // hash starts at 5381 +} + +TEST(ShardManagerTests, ComputeShardWithNumShards) { + Value key = Value::make_int64(42); + EXPECT_EQ(ShardManager::compute_shard(key, 4), ShardManager::compute_shard(key, 4)); +} + +TEST(ShardManagerTests, ComputeShardZeroShards) { + Value key = Value::make_int64(100); + // Should return 0 (not crash) when num_shards is 0 + EXPECT_EQ(ShardManager::compute_shard(key, 0), 0u); +} + +TEST(ShardManagerTests, ComputeShardDeterministic) { + Value key1 = Value::make_int64(1000); + Value key2 = Value::make_int64(1000); + uint32_t shard1 = ShardManager::compute_shard(key1, 8); + uint32_t shard2 = ShardManager::compute_shard(key2, 8); + EXPECT_EQ(shard1, shard2); +} + +TEST(ShardManagerTests, ComputeShardInRange) { + Value key = Value::make_int64(999); + uint32_t num_shards = 16; + uint32_t shard = ShardManager::compute_shard(key, num_shards); + EXPECT_LT(shard, num_shards); +} + +TEST(ShardManagerTests, GetTargetNodeEmptyShards) { + TableInfo info; + info.shards = {}; + auto result = ShardManager::get_target_node(info, 0); + EXPECT_FALSE(result.has_value()); +} + +TEST(ShardManagerTests, GetTargetNodeFound) { + ShardInfo shard; + shard.shard_id = 5; + shard.node_address = "127.0.0.1"; + shard.port = 7000; + + TableInfo info; + info.shards = {shard}; + + auto result = ShardManager::get_target_node(info, 5); + EXPECT_TRUE(result.has_value()); + EXPECT_EQ(result->node_address, "127.0.0.1"); +} + +TEST(ShardManagerTests, GetTargetNodeNotFound) { + ShardInfo shard; + shard.shard_id = 3; + shard.node_address = "127.0.0.1"; + shard.port = 7000; + + TableInfo info; + info.shards = {shard}; + + auto result = ShardManager::get_target_node(info, 99); // Different shard_id + EXPECT_FALSE(result.has_value()); +} + +// ============= DistributedExecutor Basic Tests ============= + +class DistributedExecutorTests : public ::testing::Test { + protected: + void SetUp() override { + catalog_ = Catalog::create(); + config_.mode = config::RunMode::Coordinator; + cm_ = std::make_unique(&config_); + exec_ = std::make_unique(*catalog_, *cm_); + } + + std::shared_ptr catalog_; + config::Config config_; + std::unique_ptr cm_; + std::unique_ptr exec_; +}; + +TEST_F(DistributedExecutorTests, ConstructorBasic) { + EXPECT_NE(exec_, nullptr); +} + +// DDL operations succeed because they update the local catalog +// (no distributed coordination needed for schema changes) +TEST_F(DistributedExecutorTests, ExecuteDDLWithoutNodes) { + auto lexer = std::make_unique("CREATE TABLE test_table (id INT, name TEXT)"); + Parser parser(std::move(lexer)); + auto stmt = parser.parse_statement(); + ASSERT_NE(stmt, nullptr); + + auto res = exec_->execute(*stmt, "CREATE TABLE test_table (id INT, name TEXT)"); + EXPECT_TRUE(res.success()); +} + +// DDL without nodes succeeds (local catalog update only) +TEST_F(DistributedExecutorTests, ExecuteDDLNoNodesDropTable) { + auto lexer = std::make_unique("DROP TABLE test_table"); + Parser parser(std::move(lexer)); + auto stmt = parser.parse_statement(); + ASSERT_NE(stmt, nullptr); + + auto res = exec_->execute(*stmt, "DROP TABLE test_table"); + EXPECT_TRUE(res.success()); +} + +// DML fails when no nodes because it needs shard routing +TEST_F(DistributedExecutorTests, ExecuteDMLWithoutNodes) { + auto lexer = std::make_unique("INSERT INTO test_table VALUES (1, 'test')"); + Parser parser(std::move(lexer)); + auto stmt = parser.parse_statement(); + ASSERT_NE(stmt, nullptr); + + auto res = exec_->execute(*stmt, "INSERT INTO test_table VALUES (1, 'test')"); + EXPECT_FALSE(res.success()); + EXPECT_STREQ(res.error().c_str(), "No active data nodes in cluster"); +} + +// SELECT fails when no nodes available +TEST_F(DistributedExecutorTests, ExecuteSELECTWithoutNodes) { + auto lexer = std::make_unique("SELECT * FROM test_table"); + Parser parser(std::move(lexer)); + auto stmt = parser.parse_statement(); + ASSERT_NE(stmt, nullptr); + + auto res = exec_->execute(*stmt, "SELECT * FROM test_table"); + EXPECT_FALSE(res.success()); + EXPECT_STREQ(res.error().c_str(), "No active data nodes in cluster"); +} + +// Transaction control fails when no nodes +TEST_F(DistributedExecutorTests, ExecuteBEGINWithoutNodes) { + auto lexer = std::make_unique("BEGIN"); + Parser parser(std::move(lexer)); + auto stmt = parser.parse_statement(); + ASSERT_NE(stmt, nullptr); + + auto res = exec_->execute(*stmt, "BEGIN"); + EXPECT_FALSE(res.success()); + EXPECT_STREQ(res.error().c_str(), "No active data nodes in cluster"); +} + +TEST_F(DistributedExecutorTests, ExecuteCOMMITWithoutNodes) { + auto lexer = std::make_unique("COMMIT"); + Parser parser(std::move(lexer)); + auto stmt = parser.parse_statement(); + ASSERT_NE(stmt, nullptr); + + auto res = exec_->execute(*stmt, "COMMIT"); + EXPECT_FALSE(res.success()); + EXPECT_STREQ(res.error().c_str(), "No active data nodes in cluster"); +} + +TEST_F(DistributedExecutorTests, ExecuteROLLBACKWithoutNodes) { + auto lexer = std::make_unique("ROLLBACK"); + Parser parser(std::move(lexer)); + auto stmt = parser.parse_statement(); + ASSERT_NE(stmt, nullptr); + + auto res = exec_->execute(*stmt, "ROLLBACK"); + EXPECT_FALSE(res.success()); + EXPECT_STREQ(res.error().c_str(), "No active data nodes in cluster"); +} + +// SELECT without FROM clause - parser error +TEST_F(DistributedExecutorTests, ExecuteUnknownStatementType) { + // SELECT 1 without FROM is not valid SQL in this parser + auto lexer = std::make_unique("SELECT *"); + Parser parser(std::move(lexer)); + auto stmt = parser.parse_statement(); + // Parser should fail on "SELECT *" without table + ASSERT_EQ(stmt, nullptr); +} + +// ============= Expression Sharding Key Extraction Tests ============= + +class ShardingKeyExtractionTests : public ::testing::Test { + protected: + void SetUp() override {} +}; + +TEST_F(ShardingKeyExtractionTests, ExtractShardingKeySimpleEq) { + // Test: id = 42 + auto lexer = std::make_unique("SELECT * FROM test WHERE id = 42"); + Parser parser(std::move(lexer)); + auto stmt = parser.parse_statement(); + ASSERT_NE(stmt, nullptr); + + auto* select_stmt = dynamic_cast(stmt.get()); + ASSERT_NE(select_stmt, nullptr); + ASSERT_NE(select_stmt->where(), nullptr); + + Value extracted; + // This tests the helper function in distributed_executor.cpp + // We can't call it directly, but we can test the behavior through execute + EXPECT_NE(select_stmt->where(), nullptr); +} + +TEST_F(ShardingKeyExtractionTests, NoWHEREClause) { + auto lexer = std::make_unique("SELECT * FROM test"); + Parser parser(std::move(lexer)); + auto stmt = parser.parse_statement(); + ASSERT_NE(stmt, nullptr); + + auto* select_stmt = dynamic_cast(stmt.get()); + ASSERT_NE(select_stmt, nullptr); + EXPECT_EQ(select_stmt->where(), nullptr); +} + +TEST_F(ShardingKeyExtractionTests, NonEqCondition) { + auto lexer = std::make_unique("SELECT * FROM test WHERE id > 42"); + Parser parser(std::move(lexer)); + auto stmt = parser.parse_statement(); + ASSERT_NE(stmt, nullptr); + + auto* select_stmt = dynamic_cast(stmt.get()); + ASSERT_NE(select_stmt, nullptr); + EXPECT_NE(select_stmt->where(), nullptr); +} + +// ============= Helper Function Tests ============= + +TEST(HelperTests, StableHashAlgorithm) { + // DJB2 hash algorithm verification + std::string input = "hello"; + uint32_t hash = ShardManager::stable_hash(input); + + // Manually verify DJB2: hash = hash * 33 + c for each char + uint32_t expected = 5381; + for (char c : input) { + expected = ((expected << 5) + expected) + static_cast(c); + } + EXPECT_EQ(hash, expected); +} + +TEST(HelperTests, ComputeShardModuloProperties) { + // Verify compute_shard uses modulo correctly + Value key = Value::make_int64(12345); + uint32_t shard1 = ShardManager::compute_shard(key, 10); + uint32_t shard2 = ShardManager::compute_shard(key, 10); + + // Same key, same num_shards should always give same result + EXPECT_EQ(shard1, shard2); + + // Should be in range [0, 10) + EXPECT_LT(shard1, 10); +} + +TEST(HelperTests, ComputeShardStringKey) { + // Test with string value key + Value key = Value::make_text("primary_key_value"); + uint32_t shard = ShardManager::compute_shard(key, 8); + + // Should be in range [0, 8) + EXPECT_LT(shard, 8); + EXPECT_GE(shard, 0); +} + +// ============= Null Safety Tests ============= + +TEST(NullSafetyTests, ExecuteWithEmptyCluster) { + auto catalog = Catalog::create(); + config::Config config; + ClusterManager cm(&config); + DistributedExecutor exec(*catalog, cm); + + // DDL succeeds (local catalog update), DML/SELECT fail + std::vector> statements = { + {"CREATE TABLE t (id INT)", true}, // succeeds - local catalog + {"DROP TABLE t", true}, // succeeds - local catalog + {"INSERT INTO t VALUES (1)", false}, // fails - needs nodes + {"SELECT * FROM t", false}, // fails - needs nodes + {"UPDATE t SET id = 1", false}, // fails - needs nodes + {"DELETE FROM t", false}, // fails - needs nodes + {"BEGIN", false}, // fails - needs nodes + {"COMMIT", false}, // fails - needs nodes + {"ROLLBACK", false}}; // fails - needs nodes + + for (const auto& [sql, expected_success] : statements) { + auto lexer = std::make_unique(sql); + Parser parser(std::move(lexer)); + auto stmt = parser.parse_statement(); + if (stmt) { + auto res = exec.execute(*stmt, sql); + EXPECT_EQ(res.success(), expected_success) << "Failed for: " << sql; + } + } +} + +} // namespace From 9aaf4cce54d1fffa44a117092829eda16f4ef39b Mon Sep 17 00:00:00 2001 From: poyrazK <83272398+poyrazK@users.noreply.github.com> Date: Sat, 11 Apr 2026 16:32:51 +0000 Subject: [PATCH 2/3] style: automated clang-format fixes --- tests/distributed_executor_tests.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/distributed_executor_tests.cpp b/tests/distributed_executor_tests.cpp index 7b6372f8..4e2c90aa 100644 --- a/tests/distributed_executor_tests.cpp +++ b/tests/distributed_executor_tests.cpp @@ -114,7 +114,7 @@ TEST(ShardManagerTests, GetTargetNodeNotFound) { // ============= DistributedExecutor Basic Tests ============= class DistributedExecutorTests : public ::testing::Test { - protected: + protected: void SetUp() override { catalog_ = Catalog::create(); config_.mode = config::RunMode::Coordinator; @@ -226,7 +226,7 @@ TEST_F(DistributedExecutorTests, ExecuteUnknownStatementType) { // ============= Expression Sharding Key Extraction Tests ============= class ShardingKeyExtractionTests : public ::testing::Test { - protected: + protected: void SetUp() override {} }; @@ -317,13 +317,13 @@ TEST(NullSafetyTests, ExecuteWithEmptyCluster) { // DDL succeeds (local catalog update), DML/SELECT fail std::vector> statements = { - {"CREATE TABLE t (id INT)", true}, // succeeds - local catalog + {"CREATE TABLE t (id INT)", true}, // succeeds - local catalog {"DROP TABLE t", true}, // succeeds - local catalog {"INSERT INTO t VALUES (1)", false}, // fails - needs nodes - {"SELECT * FROM t", false}, // fails - needs nodes - {"UPDATE t SET id = 1", false}, // fails - needs nodes + {"SELECT * FROM t", false}, // fails - needs nodes + {"UPDATE t SET id = 1", false}, // fails - needs nodes {"DELETE FROM t", false}, // fails - needs nodes - {"BEGIN", false}, // fails - needs nodes + {"BEGIN", false}, // fails - needs nodes {"COMMIT", false}, // fails - needs nodes {"ROLLBACK", false}}; // fails - needs nodes From 81a27a0a57f1064921b3dff98a8366f4da1fc85f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Poyraz=20K=C3=BC=C3=A7=C3=BCkarslan?= <83272398+PoyrazK@users.noreply.github.com> Date: Sun, 12 Apr 2026 17:47:58 +0300 Subject: [PATCH 3/3] Fix CodeRabbit review inline comments in distributed_executor_tests - Rename test to ParseRejectsSelectWithoutFrom (was ExecuteUnknownStatementType) - Remove tautological EXPECT_GE(shard, 0) since shard is uint32_t - Add ASSERT_TRUE(stmt) with failure message in ExecuteWithEmptyCluster loop - Update sharding key extraction tests to verify actual key values: - ExtractShardingKeySimpleEq now verifies column name "id", value 42, and op Eq - NonEqCondition now verifies op is Gt (not Eq) for WHERE id > 42 --- tests/distributed_executor_tests.cpp | 41 +++++++++++++++++++--------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/tests/distributed_executor_tests.cpp b/tests/distributed_executor_tests.cpp index 4e2c90aa..173e1c5a 100644 --- a/tests/distributed_executor_tests.cpp +++ b/tests/distributed_executor_tests.cpp @@ -214,8 +214,8 @@ TEST_F(DistributedExecutorTests, ExecuteROLLBACKWithoutNodes) { } // SELECT without FROM clause - parser error -TEST_F(DistributedExecutorTests, ExecuteUnknownStatementType) { - // SELECT 1 without FROM is not valid SQL in this parser +TEST_F(DistributedExecutorTests, ParseRejectsSelectWithoutFrom) { + // SELECT * without FROM is not valid SQL in this parser auto lexer = std::make_unique("SELECT *"); Parser parser(std::move(lexer)); auto stmt = parser.parse_statement(); @@ -239,12 +239,19 @@ TEST_F(ShardingKeyExtractionTests, ExtractShardingKeySimpleEq) { auto* select_stmt = dynamic_cast(stmt.get()); ASSERT_NE(select_stmt, nullptr); - ASSERT_NE(select_stmt->where(), nullptr); + auto* where_expr = dynamic_cast(select_stmt->where()); + ASSERT_NE(where_expr, nullptr); - Value extracted; - // This tests the helper function in distributed_executor.cpp - // We can't call it directly, but we can test the behavior through execute - EXPECT_NE(select_stmt->where(), nullptr); + // Verify it's: id = 42 + auto* left_col = dynamic_cast(&where_expr->left()); + ASSERT_NE(left_col, nullptr); + EXPECT_EQ(left_col->name(), "id"); + + auto* right_const = dynamic_cast(&where_expr->right()); + ASSERT_NE(right_const, nullptr); + EXPECT_EQ(right_const->value(), Value::make_int64(42)); + + EXPECT_EQ(where_expr->op(), TokenType::Eq); } TEST_F(ShardingKeyExtractionTests, NoWHEREClause) { @@ -259,6 +266,7 @@ TEST_F(ShardingKeyExtractionTests, NoWHEREClause) { } TEST_F(ShardingKeyExtractionTests, NonEqCondition) { + // WHERE id > 42 uses Greater operator, not equality - no valid sharding key auto lexer = std::make_unique("SELECT * FROM test WHERE id > 42"); Parser parser(std::move(lexer)); auto stmt = parser.parse_statement(); @@ -266,7 +274,16 @@ TEST_F(ShardingKeyExtractionTests, NonEqCondition) { auto* select_stmt = dynamic_cast(stmt.get()); ASSERT_NE(select_stmt, nullptr); - EXPECT_NE(select_stmt->where(), nullptr); + auto* where_expr = dynamic_cast(select_stmt->where()); + ASSERT_NE(where_expr, nullptr); + + // Verify it's: id > 42 (Greater, not Eq) + auto* left_col = dynamic_cast(&where_expr->left()); + ASSERT_NE(left_col, nullptr); + EXPECT_EQ(left_col->name(), "id"); + + // op should be Gt, not Eq - cannot extract sharding key from inequality + EXPECT_EQ(where_expr->op(), TokenType::Gt); } // ============= Helper Function Tests ============= @@ -304,7 +321,6 @@ TEST(HelperTests, ComputeShardStringKey) { // Should be in range [0, 8) EXPECT_LT(shard, 8); - EXPECT_GE(shard, 0); } // ============= Null Safety Tests ============= @@ -331,10 +347,9 @@ TEST(NullSafetyTests, ExecuteWithEmptyCluster) { auto lexer = std::make_unique(sql); Parser parser(std::move(lexer)); auto stmt = parser.parse_statement(); - if (stmt) { - auto res = exec.execute(*stmt, sql); - EXPECT_EQ(res.success(), expected_success) << "Failed for: " << sql; - } + ASSERT_TRUE(stmt) << "Parse failed for: " << sql; + auto res = exec.execute(*stmt, sql); + EXPECT_EQ(res.success(), expected_success) << "Failed for: " << sql; } }