Skip to content

Commit

Permalink
refactor: simplify DroppedId. Replace manual comparison with string…
Browse files Browse the repository at this point in the history 
… comparison (#16495)
  • Loading branch information
@drmingdrmer
drmingdrmer authored Sep 23, 2024
1 parent 3b49735 commit 163a04a
Show file tree
Hide file tree
Showing 6 changed files with 133 additions and 94 deletions.
18 changes: 12 additions & 6 deletions src/meta/api/src/schema_api_impl.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2753,7 +2753,7 @@ impl<KV: kvapi::KVApi<Error = MetaError> + ?Sized> SchemaApi for KV {
});
} else {
for (table_info, db_id) in table_infos.iter().take(capacity) {
vacuum_ids.push(DroppedId::Table(
vacuum_ids.push(DroppedId::new_table(
*db_id,
table_info.ident.table_id,
table_info.name.clone(),
Expand Down Expand Up @@ -2803,7 +2803,7 @@ impl<KV: kvapi::KVApi<Error = MetaError> + ?Sized> SchemaApi for KV {
let mut drop_table_infos = vec![];

for (table_info, db_id) in table_infos.iter().take(the_limit) {
drop_ids.push(DroppedId::Table(
drop_ids.push(DroppedId::new_table(
*db_id,
table_info.ident.table_id,
table_info.name.clone(),
Expand All @@ -2826,8 +2826,15 @@ impl<KV: kvapi::KVApi<Error = MetaError> + ?Sized> SchemaApi for KV {
db_name,
tables: _,
} => gc_dropped_db_by_id(self, db_id, &req.tenant, db_name).await?,
DroppedId::Table(db_id, table_id, table_name) => {
gc_dropped_table_by_id(self, &req.tenant, db_id, table_id, table_name).await?
DroppedId::Table { name, id } => {
gc_dropped_table_by_id(
self,
&req.tenant,
name.db_id,
id.table_id,
name.table_name.clone(),
)
.await?
}
}
}
Expand Down Expand Up @@ -3613,8 +3620,7 @@ async fn batch_filter_table_info(
.iter()
.map(|(_f, _db, table_id, _table_name)| TableId::new(*table_id));

let strm = kv_api.get_pb_values(table_id_idents).await?;
let seq_metas = strm.try_collect::<Vec<_>>().await?;
let seq_metas = kv_api.get_pb_values_vec(table_id_idents).await?;

for (seq_meta, (filter, db_info, table_id, table_name)) in seq_metas
.into_iter()
Expand Down
137 changes: 55 additions & 82 deletions src/meta/api/src/schema_api_test_suite.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
// limitations under the License.

use std::assert_ne;
use std::cmp::Ordering;
use std::collections::BTreeMap;
use std::collections::BTreeSet;
use std::collections::HashMap;
Expand Down Expand Up @@ -290,7 +289,8 @@ impl SchemaApiTestSuite {
suite.database_list(&b.build().await).await?;
suite.database_list_in_diff_tenant(&b.build().await).await?;
suite.database_rename(&b.build().await).await?;
suite.get_tenant_history_database(&b.build().await).await?;

suite.get_tenant_history_databases(&b.build().await).await?;
suite
.database_drop_undrop_list_history(&b.build().await)
.await?;
Expand Down Expand Up @@ -1089,11 +1089,11 @@ impl SchemaApiTestSuite {
}

#[fastrace::trace]
async fn get_tenant_history_database<MT: SchemaApi + kvapi::AsKVApi<Error = MetaError>>(
async fn get_tenant_history_databases<MT: SchemaApi + kvapi::AsKVApi<Error = MetaError>>(
&self,
mt: &MT,
) -> anyhow::Result<()> {
let tenant_name = "tenant_get_tenant_history_database";
let tenant_name = "tenant_get_tenant_history_databases";
let db_name_1 = "db1_get_tenant_history_database";
let db_name_2 = "db2_get_tenant_history_database";

Expand Down Expand Up @@ -4163,7 +4163,7 @@ impl SchemaApiTestSuite {
as_dropped: false,
};
let resp = mt.create_table(req.clone()).await?;
drop_ids_1.push(DroppedId::Table(
drop_ids_1.push(DroppedId::new_table(
*res.db_id,
resp.table_id,
table_name.table_name.clone(),
Expand Down Expand Up @@ -4270,8 +4270,16 @@ impl SchemaApiTestSuite {
as_dropped: false,
};
let resp = mt.create_table(req.clone()).await?;
drop_ids_1.push(DroppedId::Table(*db_id, resp.table_id, "tb1".to_string()));
drop_ids_2.push(DroppedId::Table(*db_id, resp.table_id, "tb1".to_string()));
drop_ids_1.push(DroppedId::new_table(
*db_id,
resp.table_id,
"tb1".to_string(),
));
drop_ids_2.push(DroppedId::new_table(
*db_id,
resp.table_id,
"tb1".to_string(),
));
mt.drop_table_by_id(DropTableByIdReq {
if_exists: false,
tenant: req.name_ident.tenant.clone(),
Expand Down Expand Up @@ -4299,7 +4307,11 @@ impl SchemaApiTestSuite {
as_dropped: false,
};
let resp = mt.create_table(req.clone()).await?;
drop_ids_2.push(DroppedId::Table(*db_id, resp.table_id, "tb2".to_string()));
drop_ids_2.push(DroppedId::new_table(
*db_id,
resp.table_id,
"tb2".to_string(),
));
mt.drop_table_by_id(DropTableByIdReq {
if_exists: false,
tenant: req.name_ident.tenant.clone(),
Expand Down Expand Up @@ -4334,61 +4346,6 @@ impl SchemaApiTestSuite {
}
}

fn cmp_dropped_id(l: &DroppedId, r: &DroppedId) -> Ordering {
match (l, r) {
(
DroppedId::Table(left_db_id, left_table_id, _),
DroppedId::Table(right_db_id, right_table_id, _),
) => {
if left_db_id != right_db_id {
left_db_id.cmp(right_db_id)
} else {
left_table_id.cmp(right_table_id)
}
}
(
DroppedId::Db {
db_id: left_db_id, ..
},
DroppedId::Db {
db_id: right_db_id, ..
},
) => left_db_id.cmp(right_db_id),
(
DroppedId::Db {
db_id: left_db_id,
db_name: _,
tables: _,
},
DroppedId::Table(right_db_id, _, _),
) => left_db_id.cmp(right_db_id),
(
DroppedId::Table(left_db_id, _, _),
DroppedId::Db {
db_id: right_db_id,
db_name: _,
tables: _,
},
) => left_db_id.cmp(right_db_id),
}
}
fn is_dropped_id_eq(l: &DroppedId, r: &DroppedId) -> bool {
match (l, r) {
(
DroppedId::Db {
db_id: left_db_id,
db_name: left_db_name,
tables: _,
},
DroppedId::Db {
db_id: right_db_id,
db_name: right_db_name,
tables: _,
},
) => left_db_id == right_db_id && left_db_name == right_db_name,
_ => l == r,
}
}
// case 1: test AllDroppedTables with filter time
{
let now = Utc::now();
Expand All @@ -4398,13 +4355,19 @@ impl SchemaApiTestSuite {
limit: None,
};
let resp = mt.get_drop_table_infos(req).await?;
// sort drop id by table id
let mut sort_drop_ids = resp.drop_ids;
sort_drop_ids.sort_by(cmp_dropped_id);
assert_eq!(sort_drop_ids.len(), drop_ids_1.len());
for (id1, id2) in sort_drop_ids.iter().zip(drop_ids_1.iter()) {
assert!(is_dropped_id_eq(id1, id2));
}

let got = resp
.drop_ids
.iter()
.map(|x| x.cmp_key())
.collect::<BTreeSet<_>>();

let want = drop_ids_1
.iter()
.map(|x| x.cmp_key())
.collect::<BTreeSet<_>>();

assert_eq!(got, want);

let expected: BTreeSet<String> = [
"'db1'.'tb1'".to_string(),
Expand All @@ -4430,13 +4393,19 @@ impl SchemaApiTestSuite {
limit: None,
};
let resp = mt.get_drop_table_infos(req).await?;
// sort drop id by table id
let mut sort_drop_ids = resp.drop_ids;
sort_drop_ids.sort_by(cmp_dropped_id);
assert_eq!(sort_drop_ids.len(), drop_ids_2.len());
for (id1, id2) in sort_drop_ids.iter().zip(drop_ids_2.iter()) {
assert!(is_dropped_id_eq(id1, id2));
}

let got = resp
.drop_ids
.iter()
.map(|x| x.cmp_key())
.collect::<BTreeSet<_>>();

let want = drop_ids_2
.iter()
.map(|x| x.cmp_key())
.collect::<BTreeSet<_>>();

assert_eq!(got, want);

let expected: BTreeSet<String> = [
"'db1'.'tb1'".to_string(),
Expand Down Expand Up @@ -4512,7 +4481,11 @@ impl SchemaApiTestSuite {
};
let resp = mt.create_table(req.clone()).await?;

drop_ids.push(DroppedId::Table(db_id, resp.table_id, table_name.clone()));
drop_ids.push(DroppedId::new_table(
db_id,
resp.table_id,
table_name.clone(),
));

mt.drop_table_by_id(DropTableByIdReq {
if_exists: false,
Expand Down Expand Up @@ -4644,17 +4617,17 @@ impl SchemaApiTestSuite {
let drop_ids_set: HashSet<u64> = drop_ids
.iter()
.map(|l| {
if let DroppedId::Table(_, table_id, _) = l {
*table_id
if let DroppedId::Table { name: _, id } = l {
id.table_id
} else {
unreachable!()
}
})
.collect();

for id in resp.drop_ids {
if let DroppedId::Table(_, table_id, _) = id {
assert!(drop_ids_set.contains(&table_id));
if let DroppedId::Table { name: _, id } = id {
assert!(drop_ids_set.contains(&id.table_id));
} else {
unreachable!()
}
Expand Down
34 changes: 32 additions & 2 deletions src/meta/app/src/schema/table.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,15 @@ pub struct DBIdTableName {
pub table_name: String,
}

impl DBIdTableName {
pub fn new(db_id: u64, table_name: impl ToString) -> Self {
DBIdTableName {
db_id,
table_name: table_name.to_string(),
}
}
}

impl Display for DBIdTableName {
fn fmt(&self, f: &mut Formatter) -> fmt::Result {
write!(f, "{}.'{}'", self.db_id, self.table_name)
Expand Down Expand Up @@ -934,8 +943,29 @@ pub enum DroppedId {
db_name: String,
tables: Vec<(u64, String)>,
},
// db id, table id, table name
Table(u64, u64, String),
Table {
name: DBIdTableName,
id: TableId,
},
}

impl DroppedId {
pub fn new_table(db_id: u64, table_id: u64, table_name: impl ToString) -> DroppedId {
DroppedId::Table {
name: DBIdTableName::new(db_id, table_name),
id: TableId::new(table_id),
}
}

/// Build a string contains essential information for comparison.
///
/// Only used for testing.
pub fn cmp_key(&self) -> String {
match self {
DroppedId::Db { db_id, db_name, .. } => format!("db:{}-{}", db_id, db_name),
DroppedId::Table { name, id } => format!("table:{:?}-{:?}", name, id),
}
}
}

#[derive(Clone, Debug, PartialEq, Eq)]
Expand Down
1 change: 1 addition & 0 deletions src/meta/kvapi/src/kvapi/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ pub use message::MGetKVReply;
pub use message::MGetKVReq;
pub use message::UpsertKVReply;
pub use message::UpsertKVReq;
pub use pair::BasicPair;
pub use pair::Pair;
pub use pair::SeqPair;
pub use prefix::prefix_to_range;
Expand Down
29 changes: 29 additions & 0 deletions src/meta/kvapi/src/kvapi/pair.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,35 @@ use databend_common_meta_types::SeqV;

use crate::kvapi;

/// A Key-Value pair for type Key. The value does not have a seq number.
#[derive(Clone, Debug, PartialEq, Eq)]
pub struct BasicPair<K>
where K: kvapi::Key
{
key: K,
value: K::ValueType,
}

impl<K> BasicPair<K>
where K: kvapi::Key
{
pub fn new(key: K, value: K::ValueType) -> Self {
Self { key, value }
}

pub fn key(&self) -> &K {
&self.key
}

pub fn value(&self) -> &K::ValueType {
&self.value
}

pub fn unpack(self) -> (K, K::ValueType) {
(self.key, self.value)
}
}

/// A Key-Value pair for type Key. The value has a seq number.
pub struct Pair<K>
where K: kvapi::Key
Expand Down
8 changes: 4 additions & 4 deletions src/query/service/src/interpreters/interpreter_vacuum_drop_tables.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ impl VacuumDropTablesInterpreter {
DroppedId::Db { .. } => {
drop_db_ids.push(drop_id);
}
DroppedId::Table(_, _, _) => {
DroppedId::Table { .. } => {
drop_db_table_ids.push(drop_id);
}
}
Expand Down Expand Up @@ -183,7 +183,7 @@ impl Interpreter for VacuumDropTablesInterpreter {
} else {
for (table_id, table_name) in tables.iter() {
if !failed_tables.contains(table_id) {
success_dropped_ids.push(DroppedId::Table(
success_dropped_ids.push(DroppedId::new_table(
*db_id,
*table_id,
table_name.clone(),
Expand All @@ -192,8 +192,8 @@ impl Interpreter for VacuumDropTablesInterpreter {
}
}
}
DroppedId::Table(_, table_id, _) => {
if !failed_tables.contains(table_id) {
DroppedId::Table { name: _, id } => {
if !failed_tables.contains(&id.table_id) {
success_dropped_ids.push(drop_id);
}
}
Expand Down

0 comments on commit 163a04a

Please sign in to comment.