Skip to content

Commit d5647d1

Browse files
dl239dl239
authored
fix: unsupport delete with multi index (#3374)
1 parent d67e761 commit d5647d1

File tree

12 files changed

+190
-40
lines changed

12 files changed

+190
-40
lines changed

‎src/client/tablet_client.cc

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -845,7 +845,7 @@ bool TabletClient::Delete(uint32_t tid, uint32_t pid, const std::string& pk, con
845845
}
846846

847847
base::Status TabletClient::Delete(uint32_t tid, uint32_t pid, const std::map<uint32_t, std::string>& index_val,
848-
const std::optional<uint64_t> start_ts, const std::optional<uint64_t>& end_ts) {
848+
const std::string& ts_name, const std::optional<uint64_t> start_ts, const std::optional<uint64_t>& end_ts) {
849849
::openmldb::api::DeleteRequest request;
850850
::openmldb::api::GeneralResponse response;
851851
request.set_tid(tid);
@@ -861,6 +861,9 @@ base::Status TabletClient::Delete(uint32_t tid, uint32_t pid, const std::map<uin
861861
if (end_ts.has_value()) {
862862
request.set_end_ts(end_ts.value());
863863
}
864+
if (!ts_name.empty()) {
865+
request.set_ts_name(ts_name);
866+
}
864867
bool ok = client_.SendRequest(&::openmldb::api::TabletServer_Stub::Delete, &request, &response,
865868
FLAGS_request_timeout_ms, 1);
866869
if (!ok || response.code() != 0) {

‎src/client/tablet_client.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ class TabletClient : public Client {
9494
std::string& msg); // NOLINT
9595

9696
base::Status Delete(uint32_t tid, uint32_t pid, const std::map<uint32_t, std::string>& index_val,
97-
const std::optional<uint64_t> start_ts, const std::optional<uint64_t>& end_ts);
97+
const std::string& ts_name, const std::optional<uint64_t> start_ts, const std::optional<uint64_t>& end_ts);
9898

9999
bool Count(uint32_t tid, uint32_t pid, const std::string& pk, const std::string& idx_name, bool filter_expired_data,
100100
uint64_t& value, std::string& msg); // NOLINT

‎src/cmd/sql_cmd_test.cc

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -989,6 +989,40 @@ TEST_P(DBSDKTest, DeletetRange) {
989989
});
990990
}
991991

992+
TEST_P(DBSDKTest, DeletetSameColIndex) {
993+
auto cli = GetParam();
994+
sr = cli->sr;
995+
std::string db_name = "test2";
996+
std::string table_name = "test1";
997+
std::string ddl = "create table test1 (c1 string, c2 int, c3 bigint, c4 bigint, "
998+
"INDEX(KEY=c1, ts=c3), INDEX(KEY=c1, ts=c4));";
999+
ProcessSQLs(sr, {
1000+
"set @@execute_mode = 'online'",
1001+
absl::StrCat("create database ", db_name, ";"),
1002+
absl::StrCat("use ", db_name, ";"),
1003+
ddl,
1004+
});
1005+
hybridse::sdk::Status status;
1006+
for (int i = 0; i < 10; i++) {
1007+
std::string key = absl::StrCat("key", i);
1008+
for (int j = 0; j < 10; j++) {
1009+
uint64_t ts = 1000 + j;
1010+
sr->ExecuteSQL(absl::StrCat("insert into ", table_name, " values ('", key, "', 11, ", ts, ",", ts, ");"),
1011+
&status);
1012+
}
1013+
}
1014+
1015+
auto res = sr->ExecuteSQL(absl::StrCat("select * from ", table_name, ";"), &status);
1016+
ASSERT_EQ(res->Size(), 100);
1017+
ProcessSQLs(sr, {absl::StrCat("delete from ", table_name, " where c1 = 'key2';")});
1018+
res = sr->ExecuteSQL(absl::StrCat("select * from ", table_name, ";"), &status);
1019+
ASSERT_EQ(res->Size(), 90);
1020+
ProcessSQLs(sr, {
1021+
absl::StrCat("drop table ", table_name),
1022+
absl::StrCat("drop database ", db_name),
1023+
});
1024+
}
1025+
9921026
TEST_P(DBSDKTest, SQLDeletetRow) {
9931027
auto cli = GetParam();
9941028
sr = cli->sr;

‎src/proto/tablet.proto

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,7 @@ message DeleteRequest {
209209
repeated Dimension dimensions = 5;
210210
optional uint64 ts = 6;
211211
optional uint64 end_ts = 7;
212+
optional string ts_name = 8;
212213
}
213214

214215
message ExecuteGcRequest {
@@ -402,6 +403,7 @@ message LogEntry {
402403
optional MethodType method_type = 7;
403404
repeated TSDimension ts_dimensions = 8 [deprecated = true];
404405
optional uint64 end_ts = 9; // only for range delete: [ts, end_ts)
406+
optional string ts_name = 10;
405407
}
406408

407409
message AppendEntriesRequest {

‎src/sdk/node_adapter.cc

Lines changed: 60 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,20 @@ hybridse::sdk::Status NodeAdapter::ExtractDeleteOption(
3939
const ::google::protobuf::RepeatedPtrField<::openmldb::common::ColumnKey>& indexs,
4040
const std::vector<Condition>& condition_vec,
4141
DeleteOption* option) {
42-
std::string con_ts_name;
42+
std::vector<::openmldb::common::ColumnKey> index_vec;
43+
std::map<std::string, uint32_t> index_pos;
4344
for (auto idx = 0; idx < indexs.size(); idx++) {
44-
const auto& column_key = indexs.Get(idx);
45+
index_vec.push_back(indexs.Get(idx));
46+
index_pos.emplace(indexs.Get(idx).index_name(), idx);
47+
}
48+
std::stable_sort(index_vec.begin(), index_vec.end(),
49+
[](const ::openmldb::common::ColumnKey& index1, const ::openmldb::common::ColumnKey& index2) {
50+
return index1.col_name_size() > index2.col_name_size();
51+
});
52+
std::string con_ts_name;
53+
::openmldb::common::ColumnKey matched_column_key;
54+
std::set<std::string> hit_con_col;
55+
for (const auto& column_key : index_vec) {
4556
if (column_key.flag() != 0) {
4657
continue;
4758
}
@@ -71,7 +82,6 @@ hybridse::sdk::Status NodeAdapter::ExtractDeleteOption(
7182
if (match_index_col != column_key.col_name_size()) {
7283
continue;
7384
}
74-
option->index_map.emplace(idx, pk);
7585
}
7686
if (column_key.has_ts_name()) {
7787
for (const auto& con : condition_vec) {
@@ -87,6 +97,10 @@ hybridse::sdk::Status NodeAdapter::ExtractDeleteOption(
8797
if (!con.val.has_value()) {
8898
return {hybridse::common::StatusCode::kCmdError, "ts column cannot be null"};
8999
}
100+
if (option->ts_name.empty()) {
101+
option->ts_name = con_ts_name;
102+
hit_con_col.insert(con_ts_name);
103+
}
90104
uint64_t ts = 0;
91105
if (!absl::SimpleAtoi(con.val.value(), &ts)) {
92106
return {hybridse::common::StatusCode::kCmdError,
@@ -114,6 +128,37 @@ hybridse::sdk::Status NodeAdapter::ExtractDeleteOption(
114128
option->end_ts.value() >= option->start_ts.value()) {
115129
return {hybridse::common::StatusCode::kCmdError, "invalid ts condition"};
116130
}
131+
} else if (!option->ts_name.empty() && match_index_col > 0) {
132+
return {hybridse::common::StatusCode::kCmdError, "ts name mismatch"};
133+
}
134+
if (match_index_col > 0) {
135+
if (!option->ts_name.empty()) {
136+
option->index_map.clear();
137+
}
138+
if (option->index_map.empty()) {
139+
matched_column_key.CopyFrom(column_key);
140+
} else {
141+
if (column_key.col_name_size() != matched_column_key.col_name_size() ||
142+
!std::equal(column_key.col_name().begin(), column_key.col_name().end(),
143+
matched_column_key.col_name().begin())) {
144+
return {hybridse::common::StatusCode::kCmdError, "hit multiple indexs"};
145+
}
146+
}
147+
option->index_map.emplace(index_pos[column_key.index_name()], pk);
148+
for (const auto& col : matched_column_key.col_name()) {
149+
hit_con_col.insert(col);
150+
}
151+
if (!option->ts_name.empty()) {
152+
break;
153+
}
154+
}
155+
}
156+
if (!option->ts_name.empty() && !option->index_map.empty() && option->ts_name != matched_column_key.ts_name()) {
157+
return {hybridse::common::StatusCode::kCmdError, "ts name mismatch"};
158+
}
159+
for (const auto& con : condition_vec) {
160+
if (hit_con_col.find(con.col_name) == hit_con_col.end()) {
161+
return {hybridse::common::StatusCode::kCmdError, "has redundant condition"};
117162
}
118163
}
119164
return {};
@@ -639,7 +684,6 @@ std::shared_ptr<hybridse::node::ConstNode> NodeAdapter::StringToData(const std::
639684

640685
hybridse::sdk::Status NodeAdapter::ParseExprNode(const hybridse::node::BinaryExpr* expr_node,
641686
const std::map<std::string, openmldb::type::DataType>& col_map,
642-
const ::google::protobuf::RepeatedPtrField<::openmldb::common::ColumnKey>& indexs,
643687
std::vector<Condition>* condition_vec, std::vector<Condition>* parameter_vec) {
644688
auto op_type = expr_node->GetOp();
645689
switch (op_type) {
@@ -650,7 +694,7 @@ hybridse::sdk::Status NodeAdapter::ParseExprNode(const hybridse::node::BinaryExp
650694
if (node == nullptr) {
651695
return {::hybridse::common::StatusCode::kCmdError, "parse expr node failed"};
652696
}
653-
auto status = ParseExprNode(node, col_map, indexs, condition_vec, parameter_vec);
697+
auto status = ParseExprNode(node, col_map, condition_vec, parameter_vec);
654698
if (!status.IsOK()) {
655699
return status;
656700
}
@@ -707,6 +751,17 @@ hybridse::sdk::Status NodeAdapter::ParseExprNode(const hybridse::node::BinaryExp
707751
return {::hybridse::common::StatusCode::kCmdError,
708752
"unsupport operator type " + hybridse::node::ExprOpTypeName(op_type)};
709753
}
754+
return {};
755+
}
756+
757+
hybridse::sdk::Status NodeAdapter::ExtractCondition(const hybridse::node::BinaryExpr* expr_node,
758+
const std::map<std::string, openmldb::type::DataType>& col_map,
759+
const ::google::protobuf::RepeatedPtrField<::openmldb::common::ColumnKey>& indexs,
760+
std::vector<Condition>* condition_vec, std::vector<Condition>* parameter_vec) {
761+
auto status = ParseExprNode(expr_node, col_map, condition_vec, parameter_vec);
762+
if (!status.IsOK()) {
763+
return status;
764+
}
710765
if (parameter_vec->empty()) {
711766
return CheckCondition(indexs, *condition_vec);
712767
} else if (condition_vec->empty()) {

‎src/sdk/node_adapter.h

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,11 +32,12 @@ namespace openmldb {
3232
namespace sdk {
3333

3434
struct DeleteOption {
35-
DeleteOption(const std::map<uint32_t, std::string>& index,
35+
DeleteOption(const std::map<uint32_t, std::string>& index, const std::string& name,
3636
const std::optional<uint64_t>& ts1, const std::optional<uint64_t>& ts2) :
37-
index_map(index), start_ts(ts1), end_ts(ts2) {}
37+
index_map(index), ts_name(name), start_ts(ts1), end_ts(ts2) {}
3838
DeleteOption() = default;
3939
std::map<uint32_t, std::string> index_map;
40+
std::string ts_name;
4041
std::optional<uint64_t> start_ts = std::nullopt;
4142
std::optional<uint64_t> end_ts = std::nullopt;
4243
};
@@ -59,7 +60,7 @@ class NodeAdapter {
5960
static std::shared_ptr<hybridse::node::ConstNode> StringToData(const std::string& str,
6061
openmldb::type::DataType data_type);
6162

62-
static hybridse::sdk::Status ParseExprNode(const hybridse::node::BinaryExpr* expr_node,
63+
static hybridse::sdk::Status ExtractCondition(const hybridse::node::BinaryExpr* expr_node,
6364
const std::map<std::string, openmldb::type::DataType>& col_map,
6465
const ::google::protobuf::RepeatedPtrField<::openmldb::common::ColumnKey>& indexs,
6566
std::vector<Condition>* condition_vec, std::vector<Condition>* parameter_vec);
@@ -72,6 +73,9 @@ class NodeAdapter {
7273
static hybridse::sdk::Status CheckCondition(
7374
const ::google::protobuf::RepeatedPtrField<::openmldb::common::ColumnKey>& indexs,
7475
const std::vector<Condition>& condition_vec);
76+
static hybridse::sdk::Status ParseExprNode(const hybridse::node::BinaryExpr* expr_node,
77+
const std::map<std::string, openmldb::type::DataType>& col_map,
78+
std::vector<Condition>* condition_vec, std::vector<Condition>* parameter_vec);
7579
};
7680

7781
} // namespace sdk

‎src/sdk/node_adapter_test.cc

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ void CheckDeleteOption(const DeleteOption& option, const DeleteOption& expect_op
130130
ASSERT_TRUE(iter != expect_option.index_map.end());
131131
ASSERT_EQ(kv.second, iter->second);
132132
}
133+
ASSERT_EQ(expect_option.ts_name, option.ts_name);
133134
if (option.start_ts.has_value()) {
134135
ASSERT_TRUE(expect_option.start_ts.has_value());
135136
ASSERT_EQ(option.start_ts.value(), expect_option.start_ts.value());
@@ -164,51 +165,51 @@ TEST_P(DeleteOptionTest, TransformToTableInfo) {
164165

165166
std::vector<DeleteOptionParm> option_cases = {
166167
DeleteOptionParm({Condition("card", hybridse::node::FnOperator::kFnOpEq, "key1", type::DataType::kString)},
167-
DeleteOption({{0, "key1"}}, std::nullopt, std::nullopt)),
168+
DeleteOption({{0, "key1"}}, "", std::nullopt, std::nullopt)),
168169
DeleteOptionParm({Condition("card", hybridse::node::FnOperator::kFnOpEq, "key1", type::DataType::kString),
169170
Condition("ts1", hybridse::node::FnOperator::kFnOpEq, "10", type::DataType::kBigInt)},
170-
DeleteOption({{0, "key1"}}, 10, 9)),
171+
DeleteOption({{0, "key1"}}, "ts1", 10, 9)),
171172
DeleteOptionParm({Condition("card", hybridse::node::FnOperator::kFnOpEq, "key1", type::DataType::kString),
172173
Condition("ts1", hybridse::node::FnOperator::kFnOpGe, "10", type::DataType::kBigInt)},
173-
DeleteOption({{0, "key1"}}, std::nullopt, 9)),
174+
DeleteOption({{0, "key1"}}, "ts1", std::nullopt, 9)),
174175
DeleteOptionParm({Condition("card", hybridse::node::FnOperator::kFnOpEq, "key1", type::DataType::kString),
175176
Condition("ts1", hybridse::node::FnOperator::kFnOpGt, "10", type::DataType::kBigInt)},
176-
DeleteOption({{0, "key1"}}, std::nullopt, 10)),
177+
DeleteOption({{0, "key1"}}, "ts1", std::nullopt, 10)),
177178
DeleteOptionParm({Condition("card", hybridse::node::FnOperator::kFnOpEq, "key1", type::DataType::kString),
178179
Condition("ts1", hybridse::node::FnOperator::kFnOpLt, "10", type::DataType::kBigInt)},
179-
DeleteOption({{0, "key1"}}, 9, std::nullopt)),
180+
DeleteOption({{0, "key1"}}, "ts1", 9, std::nullopt)),
180181
DeleteOptionParm({Condition("card", hybridse::node::FnOperator::kFnOpEq, "key1", type::DataType::kString),
181182
Condition("ts1", hybridse::node::FnOperator::kFnOpLe, "10", type::DataType::kBigInt)},
182-
DeleteOption({{0, "key1"}}, 10, std::nullopt)),
183+
DeleteOption({{0, "key1"}}, "ts1", 10, std::nullopt)),
183184
DeleteOptionParm({Condition("card", hybridse::node::FnOperator::kFnOpEq, "key1", type::DataType::kString),
184185
Condition("ts1", hybridse::node::FnOperator::kFnOpGe, "0", type::DataType::kBigInt)},
185-
DeleteOption({{0, "key1"}}, std::nullopt, std::nullopt)),
186+
DeleteOption({{0, "key1"}}, "ts1", std::nullopt, std::nullopt)),
186187
DeleteOptionParm({Condition("card", hybridse::node::FnOperator::kFnOpEq, "key1", type::DataType::kString),
187188
Condition("ts1", hybridse::node::FnOperator::kFnOpEq, "0", type::DataType::kBigInt)},
188-
DeleteOption({{0, "key1"}}, 0, std::nullopt)),
189+
DeleteOption({{0, "key1"}}, "ts1", 0, std::nullopt)),
189190
DeleteOptionParm({Condition("ts1", hybridse::node::FnOperator::kFnOpEq, "10", type::DataType::kBigInt)},
190-
DeleteOption({}, 10, 9)),
191+
DeleteOption({}, "ts1", 10, 9)),
191192
DeleteOptionParm({Condition("ts1", hybridse::node::FnOperator::kFnOpGe, "10", type::DataType::kBigInt)},
192-
DeleteOption({}, std::nullopt, 9)),
193+
DeleteOption({}, "ts1", std::nullopt, 9)),
193194
DeleteOptionParm({Condition("ts1", hybridse::node::FnOperator::kFnOpGe, "10", type::DataType::kBigInt),
194195
Condition("ts1", hybridse::node::FnOperator::kFnOpGe, "11", type::DataType::kBigInt)},
195-
DeleteOption({}, std::nullopt, 10)),
196+
DeleteOption({}, "ts1", std::nullopt, 10)),
196197
DeleteOptionParm({Condition("ts1", hybridse::node::FnOperator::kFnOpGe, "10", type::DataType::kBigInt),
197198
Condition("ts1", hybridse::node::FnOperator::kFnOpGe, "11", type::DataType::kBigInt)},
198-
DeleteOption({}, std::nullopt, 10)),
199+
DeleteOption({}, "ts1", std::nullopt, 10)),
199200
DeleteOptionParm({Condition("ts1", hybridse::node::FnOperator::kFnOpGe, "10", type::DataType::kBigInt),
200201
Condition("ts1", hybridse::node::FnOperator::kFnOpLt, "20", type::DataType::kBigInt)},
201-
DeleteOption({}, 19, 9)),
202+
DeleteOption({}, "ts1", 19, 9)),
202203
DeleteOptionParm({Condition("ts1", hybridse::node::FnOperator::kFnOpGt, "10", type::DataType::kBigInt),
203204
Condition("ts1", hybridse::node::FnOperator::kFnOpLt, "20", type::DataType::kBigInt)},
204-
DeleteOption({}, 19, 10)),
205+
DeleteOption({}, "ts1", 19, 10)),
205206
DeleteOptionParm({Condition("ts1", hybridse::node::FnOperator::kFnOpGt, "10", type::DataType::kBigInt),
206207
Condition("ts1", hybridse::node::FnOperator::kFnOpLe, "20", type::DataType::kBigInt)},
207-
DeleteOption({}, 20, 10)),
208+
DeleteOption({}, "ts1", 20, 10)),
208209
DeleteOptionParm({Condition("card", hybridse::node::FnOperator::kFnOpEq, "key1", type::DataType::kString),
209210
Condition("ts1", hybridse::node::FnOperator::kFnOpGt, "10", type::DataType::kBigInt),
210211
Condition("ts1", hybridse::node::FnOperator::kFnOpLe, "20", type::DataType::kBigInt)},
211-
DeleteOption({{0, "key1"}}, 20, 10))
212+
DeleteOption({{0, "key1"}}, "ts1", 20, 10))
212213
};
213214
INSTANTIATE_TEST_SUITE_P(NodeAdapter, DeleteOptionTest, testing::ValuesIn(option_cases));
214215

‎src/sdk/sql_cluster_router.cc

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,7 @@ std::shared_ptr<openmldb::sdk::SQLDeleteRow> SQLClusterRouter::GetDeleteRow(cons
421421
std::vector<Condition> condition_vec;
422422
std::vector<Condition> parameter_vec;
423423
auto binary_node = dynamic_cast<const hybridse::node::BinaryExpr*>(condition);
424-
*status = NodeAdapter::ParseExprNode(binary_node, col_map, table_info->column_key(),
424+
*status = NodeAdapter::ExtractCondition(binary_node, col_map, table_info->column_key(),
425425
&condition_vec, &parameter_vec);
426426
if (!status->IsOK()) {
427427
LOG(WARNING) << status->ToString();
@@ -3193,7 +3193,7 @@ hybridse::sdk::Status SQLClusterRouter::HandleDelete(const std::string& db, cons
31933193
std::vector<Condition> parameter_vec;
31943194
auto binary_node = dynamic_cast<const hybridse::node::BinaryExpr*>(condition);
31953195
auto col_map = schema::SchemaAdapter::GetColMap(*table_info);
3196-
auto status = NodeAdapter::ParseExprNode(binary_node, col_map, table_info->column_key(),
3196+
auto status = NodeAdapter::ExtractCondition(binary_node, col_map, table_info->column_key(),
31973197
&condition_vec, &parameter_vec);
31983198
if (!status.IsOK()) {
31993199
return status;
@@ -3225,7 +3225,7 @@ hybridse::sdk::Status SQLClusterRouter::SendDeleteRequst(
32253225
for (size_t idx = 0; idx < tablets.size(); idx++) {
32263226
auto tablet_client = tablets.at(idx)->GetClient();
32273227
if (auto status = tablet_client->Delete(table_info->tid(), idx,
3228-
option->index_map, option->start_ts, option->end_ts); !status.OK()) {
3228+
option->index_map, option->ts_name, option->start_ts, option->end_ts); !status.OK()) {
32293229
return {StatusCode::kCmdError, status.GetMsg()};
32303230
}
32313231
}
@@ -3248,7 +3248,8 @@ hybridse::sdk::Status SQLClusterRouter::SendDeleteRequst(
32483248
if (!tablet_client) {
32493249
return {StatusCode::kCmdError, "tablet client is null"};
32503250
}
3251-
auto ret = tablet_client->Delete(table_info->tid(), kv.first, kv.second, option->start_ts, option->end_ts);
3251+
auto ret = tablet_client->Delete(table_info->tid(), kv.first, kv.second,
3252+
option->ts_name, option->start_ts, option->end_ts);
32523253
if (!ret.OK()) {
32533254
return {StatusCode::kCmdError, ret.GetMsg()};
32543255
}

0 commit comments

Comments
 (0)