From 8cc5b9280813581f15a51b524c484c70bbe51aa2 Mon Sep 17 00:00:00 2001 From: Yuriy Dupyn <2153100+omedusyo@users.noreply.github.com> Date: Thu, 1 Feb 2024 13:25:09 +0100 Subject: [PATCH 1/7] Remove TODO's about Null. We'll implement Option in another PR --- minisql/src/type_system.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/minisql/src/type_system.rs b/minisql/src/type_system.rs index abae701..532025f 100644 --- a/minisql/src/type_system.rs +++ b/minisql/src/type_system.rs @@ -13,9 +13,6 @@ pub enum DbType { // ==============Values================ pub type Uuid = u64; -// TODO: What about nulls? I would rather not have that in SQL, it sucks. -// I would rather have non-nullable values by default, -// and something like an explicit Option type for nulls. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub enum Value { Number(f64), // TODO: Can't put floats as keys in maps, since they don't implement Eq. What to @@ -28,7 +25,6 @@ pub enum IndexableValue { String(String), Int(u64), Uuid(Uuid), - // TODO: what about null? } impl DbType { From df108f581ccdc142fd95da6d6740ba8514d116e7 Mon Sep 17 00:00:00 2001 From: Yuriy Dupyn <2153100+omedusyo@users.noreply.github.com> Date: Thu, 1 Feb 2024 13:25:43 +0100 Subject: [PATCH 2/7] Explain why floats are different with respect to indexing --- minisql/src/type_system.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/minisql/src/type_system.rs b/minisql/src/type_system.rs index 532025f..c0ae728 100644 --- a/minisql/src/type_system.rs +++ b/minisql/src/type_system.rs @@ -15,8 +15,20 @@ pub type Uuid = u64; #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub enum Value { - Number(f64), // TODO: Can't put floats as keys in maps, since they don't implement Eq. What to - // do? + // Note that it doesn't really make sense to compare floats on equality without specifying + // precision. You can ofcourse convert a float to string or to a bytevector and then compare + // equality of those, but that's not the right equality. And ofcourse Rust designers are aware + // of this, so floats don't implement the Eq trait. + // This ofcourse complicates indexing of Number columns. + // + // Either we'd have to design a specific key-value map data-structure where keys are floats, + // s.t. to index with a given float K you also specify a tolerance error so that the resulting + // value set will contain all values whose keys are close to K within that tolerence. This + // seems highly non-trivial. + // + // So we choose to make a distinction between indexable and non-indexable types, and Number is + // not indexable. + Number(f64), Indexable(IndexableValue), } From 83fd46b4f5095e8a5ef24f9b9a2682004d912b56 Mon Sep 17 00:00:00 2001 From: Yuriy Dupyn <2153100+omedusyo@users.noreply.github.com> Date: Thu, 1 Feb 2024 13:29:05 +0100 Subject: [PATCH 3/7] Explain why implementing Debug for Response::Selected(...) is problematic --- minisql/src/interpreter.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/minisql/src/interpreter.rs b/minisql/src/interpreter.rs index 773eac1..89df2b2 100644 --- a/minisql/src/interpreter.rs +++ b/minisql/src/interpreter.rs @@ -35,10 +35,9 @@ impl std::fmt::Debug for Response<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { use Response::*; match self { - Selected(_schema, _columns, _rows) => - // TODO: How can we iterate through the rows without having to take ownership of - // them? - { + Selected(_schema, _columns, _rows) => { + // It seems that Rust requires ownership of rows to format them here. + // This is why we output the string below f.write_str("Some rows... trust me") } Inserted => f.write_str("Inserted"), From 5909df60b0aa0a52f56051a2b102eb9770b1125a Mon Sep 17 00:00:00 2001 From: Yuriy Dupyn <2153100+omedusyo@users.noreply.github.com> Date: Thu, 1 Feb 2024 13:30:59 +0100 Subject: [PATCH 4/7] Explain one occurence of extra braces around a block of code in tests --- minisql/src/interpreter.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/minisql/src/interpreter.rs b/minisql/src/interpreter.rs index 89df2b2..e474673 100644 --- a/minisql/src/interpreter.rs +++ b/minisql/src/interpreter.rs @@ -574,8 +574,8 @@ pub fn example() { { { - // TODO: Why do I have to write these braces explicitely? Why doesn't Rust compiler - // "infer" them? + // HEY_BTW: Seems that there is some Rust inference feature still missing when it comes to + // ownership. That's why we put this block in braces. let _delete_response: Response = state .interpret(Delete(users_position, Some(Eq(id_column, id0.clone())))) .unwrap(); From e22b31dc415fe9682edfde32771e7fd73979dbd5 Mon Sep 17 00:00:00 2001 From: Yuriy Dupyn <2153100+omedusyo@users.noreply.github.com> Date: Thu, 1 Feb 2024 13:31:42 +0100 Subject: [PATCH 5/7] Locking per table is a concern of a different PR --- minisql/src/internals/table.rs | 3 +-- minisql/src/interpreter.rs | 1 - 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/minisql/src/internals/table.rs b/minisql/src/internals/table.rs index 0f9bb2c..66df647 100644 --- a/minisql/src/internals/table.rs +++ b/minisql/src/internals/table.rs @@ -12,8 +12,7 @@ use crate::type_system::{IndexableValue, Uuid, Value}; #[derive(Debug, Serialize, Deserialize)] pub struct Table { schema: TableSchema, - rows: Rows, // TODO: Consider wrapping this in a lock. Also consider if we need to have the - // same lock for both rows and indexes + rows: Rows, indexes: HashMap, } diff --git a/minisql/src/interpreter.rs b/minisql/src/interpreter.rs index e474673..5955c54 100644 --- a/minisql/src/interpreter.rs +++ b/minisql/src/interpreter.rs @@ -88,7 +88,6 @@ impl State { } pub fn interpret<'a>(&'a mut self, operation: Operation) -> DbResult> { - // TODO: lock stuff use Operation::*; match operation { From a3e3390c857b6a1de605d4ff0ff1565dfe492b75 Mon Sep 17 00:00:00 2001 From: Yuriy Dupyn <2153100+omedusyo@users.noreply.github.com> Date: Thu, 1 Feb 2024 13:35:10 +0100 Subject: [PATCH 6/7] Explain the role of the DESIGN_OLD.md file --- DESIGN.md => DESIGN_OLD.md | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) rename DESIGN.md => DESIGN_OLD.md (91%) diff --git a/DESIGN.md b/DESIGN_OLD.md similarity index 91% rename from DESIGN.md rename to DESIGN_OLD.md index d94f87b..b031efe 100644 --- a/DESIGN.md +++ b/DESIGN_OLD.md @@ -1,3 +1,7 @@ +Note that this is a Historical Document. It is a first attempt at +figuring out basic design requirements. + + # MiniSQL ## Official Description @@ -40,7 +44,7 @@ Possible usage: ```./minisql server start --db path/to/db/my-db.db --port 1433``` which will store the database as a file `path/to/db/my-db.db` and open a TCP server on port `1433` * Then on possibly a different machine you run `./minisql client connect server_ip_address:6666` to start a client. This will open a REPL with which you can send queries/db management commands -* TODO: We should also consider writing a rust library that allows you to spin up a client that connects to the server. +* We should also consider writing a rust library that allows you to spin up a client that connects to the server. How would the interface look like? ``` use mysql::{DB, DBConnection} @@ -69,7 +73,7 @@ which will store the database as a file `path/to/db/my-db.db` and open a TCP ser how will the parsing output look like? Consider something like ``` -// TODO: Parser has access to all table metadata +// Parser has access to all table metadata // Could also be called `SQLAbstractSyntaxTree` enum Operation { @@ -132,7 +136,7 @@ type TableName = String // Note that it is nice to split metadata from the data because // then you can give the metadata to the parser without giving it the data. struct TableMetaData { - name: TableName, // TODO: Is this really necessary? probably not + name: TableName, columns: Vec<(ColumnName, DbType, ColumnPosition)> } @@ -142,7 +146,7 @@ struct Table { meta: TableMetaData, rows: Rows // defined below indexes: - BTree // TODO: Consider generalizing ColumnName to semething that would also apply to a pair of ColumnNames etc + BTree } type Tables = HashMap @@ -183,16 +187,13 @@ Vec> * Interpreter ``` trait SqlConsumer { - // TODO: ??? } fn interpret(operation: Operation, tables: &mut Tables, consumer: T) -> () { - // TODO: lock stuff match operation { Select(table_name, column_selection, maybe_condition) => { let table: Table = ... - // TODO: Wrap this into a response select(table, column_selection, maybe_condition, consumer) }, Insert(table_name, Vec<(ColumnName, DbValue)>) => { @@ -209,7 +210,7 @@ fn interpret(operation: Operation, tables: &mut Tables, consumer enum Response { - Selected(impl Iter) // TODO: How to do this? Some reference to an iterator somehow... slice..? + Selected(impl Iter) // How to do this? Some reference to an iterator somehow... slice..? Inserted(???), Deleted(usize), // how many were deleted } @@ -221,7 +222,7 @@ fn select(table: Table, ColumnName ``` -* TODO: Consider streaming the response to the client and not just dumping 10K rows at once. +* Consider streaming the response to the client and not just dumping 10K rows at once. From 7c9caa032b7888f2134c577af42b14c600cea871 Mon Sep 17 00:00:00 2001 From: Yuriy Dupyn <2153100+omedusyo@users.noreply.github.com> Date: Thu, 1 Feb 2024 14:20:24 +0100 Subject: [PATCH 7/7] Remove HEY_BTW: comment --- minisql/src/interpreter.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/minisql/src/interpreter.rs b/minisql/src/interpreter.rs index 5955c54..09b5e18 100644 --- a/minisql/src/interpreter.rs +++ b/minisql/src/interpreter.rs @@ -573,8 +573,6 @@ pub fn example() { { { - // HEY_BTW: Seems that there is some Rust inference feature still missing when it comes to - // ownership. That's why we put this block in braces. let _delete_response: Response = state .interpret(Delete(users_position, Some(Eq(id_column, id0.clone())))) .unwrap();