Skip to content

Commit 81a27a0

Browse files
committed
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
1 parent 9aaf4cc commit 81a27a0

1 file changed

Lines changed: 28 additions & 13 deletions

File tree

tests/distributed_executor_tests.cpp

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -214,8 +214,8 @@ TEST_F(DistributedExecutorTests, ExecuteROLLBACKWithoutNodes) {
214214
}
215215

216216
// SELECT without FROM clause - parser error
217-
TEST_F(DistributedExecutorTests, ExecuteUnknownStatementType) {
218-
// SELECT 1 without FROM is not valid SQL in this parser
217+
TEST_F(DistributedExecutorTests, ParseRejectsSelectWithoutFrom) {
218+
// SELECT * without FROM is not valid SQL in this parser
219219
auto lexer = std::make_unique<Lexer>("SELECT *");
220220
Parser parser(std::move(lexer));
221221
auto stmt = parser.parse_statement();
@@ -239,12 +239,19 @@ TEST_F(ShardingKeyExtractionTests, ExtractShardingKeySimpleEq) {
239239

240240
auto* select_stmt = dynamic_cast<const SelectStatement*>(stmt.get());
241241
ASSERT_NE(select_stmt, nullptr);
242-
ASSERT_NE(select_stmt->where(), nullptr);
242+
auto* where_expr = dynamic_cast<const BinaryExpr*>(select_stmt->where());
243+
ASSERT_NE(where_expr, nullptr);
243244

244-
Value extracted;
245-
// This tests the helper function in distributed_executor.cpp
246-
// We can't call it directly, but we can test the behavior through execute
247-
EXPECT_NE(select_stmt->where(), nullptr);
245+
// Verify it's: id = 42
246+
auto* left_col = dynamic_cast<const ColumnExpr*>(&where_expr->left());
247+
ASSERT_NE(left_col, nullptr);
248+
EXPECT_EQ(left_col->name(), "id");
249+
250+
auto* right_const = dynamic_cast<const ConstantExpr*>(&where_expr->right());
251+
ASSERT_NE(right_const, nullptr);
252+
EXPECT_EQ(right_const->value(), Value::make_int64(42));
253+
254+
EXPECT_EQ(where_expr->op(), TokenType::Eq);
248255
}
249256

250257
TEST_F(ShardingKeyExtractionTests, NoWHEREClause) {
@@ -259,14 +266,24 @@ TEST_F(ShardingKeyExtractionTests, NoWHEREClause) {
259266
}
260267

261268
TEST_F(ShardingKeyExtractionTests, NonEqCondition) {
269+
// WHERE id > 42 uses Greater operator, not equality - no valid sharding key
262270
auto lexer = std::make_unique<Lexer>("SELECT * FROM test WHERE id > 42");
263271
Parser parser(std::move(lexer));
264272
auto stmt = parser.parse_statement();
265273
ASSERT_NE(stmt, nullptr);
266274

267275
auto* select_stmt = dynamic_cast<const SelectStatement*>(stmt.get());
268276
ASSERT_NE(select_stmt, nullptr);
269-
EXPECT_NE(select_stmt->where(), nullptr);
277+
auto* where_expr = dynamic_cast<const BinaryExpr*>(select_stmt->where());
278+
ASSERT_NE(where_expr, nullptr);
279+
280+
// Verify it's: id > 42 (Greater, not Eq)
281+
auto* left_col = dynamic_cast<const ColumnExpr*>(&where_expr->left());
282+
ASSERT_NE(left_col, nullptr);
283+
EXPECT_EQ(left_col->name(), "id");
284+
285+
// op should be Gt, not Eq - cannot extract sharding key from inequality
286+
EXPECT_EQ(where_expr->op(), TokenType::Gt);
270287
}
271288

272289
// ============= Helper Function Tests =============
@@ -304,7 +321,6 @@ TEST(HelperTests, ComputeShardStringKey) {
304321

305322
// Should be in range [0, 8)
306323
EXPECT_LT(shard, 8);
307-
EXPECT_GE(shard, 0);
308324
}
309325

310326
// ============= Null Safety Tests =============
@@ -331,10 +347,9 @@ TEST(NullSafetyTests, ExecuteWithEmptyCluster) {
331347
auto lexer = std::make_unique<Lexer>(sql);
332348
Parser parser(std::move(lexer));
333349
auto stmt = parser.parse_statement();
334-
if (stmt) {
335-
auto res = exec.execute(*stmt, sql);
336-
EXPECT_EQ(res.success(), expected_success) << "Failed for: " << sql;
337-
}
350+
ASSERT_TRUE(stmt) << "Parse failed for: " << sql;
351+
auto res = exec.execute(*stmt, sql);
352+
EXPECT_EQ(res.success(), expected_success) << "Failed for: " << sql;
338353
}
339354
}
340355

0 commit comments

Comments
 (0)