Merge branch 'deal_with_todos' into 'main'

Deal with todos in minisql crate

See merge request x433485/minisql!27

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<ColumnName, Index> // TODO: Consider generalizing ColumnName to semething that would also apply to a pair of ColumnNames etc
+        BTree<ColumnName, Index>
 }
 
 type Tables = HashMap<TableName, Table>
@@ -183,16 +187,13 @@ Vec<Vec<DbValue>>
 * Interpreter
 ```
 trait SqlConsumer {
-    // TODO: 
     ???
 }
 
 fn interpret<T: SqlConsumer>(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<T: SqlConsumer>(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.
 
 
 

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
+    rows: Rows,
-    // same lock for both rows and indexes
     indexes: HashMap<Column, ColumnIndex>,
 }
 

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) =>
+            Selected(_schema, _columns, _rows) => {
-            // TODO: How can we iterate through the rows without having to take ownership of
+                // It seems that Rust requires ownership of rows to format them here.
-            // them?
+                // This is why we output the string below
-            {
                 f.write_str("Some rows... trust me")
             }
             Inserted => f.write_str("Inserted"),
@@ -89,7 +88,6 @@ impl State {
     }
 
     pub fn interpret<'a>(&'a mut self, operation: Operation) -> DbResult<Response<'a>> {
-        // TODO: lock stuff
         use Operation::*;
 
         match operation {
@@ -575,8 +573,6 @@ pub fn example() {
 
     {
         {
-            // TODO: Why do I have to write these braces explicitely? Why doesn't Rust compiler
-            // "infer" them?
             let _delete_response: Response = state
                 .interpret(Delete(users_position, Some(Eq(id_column, id0.clone()))))
                 .unwrap();

minisql/src/type_system.rs

@@ -13,14 +13,23 @@ 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)]
 #[serde(try_from = "String", into = "String")]
 pub enum Value {
-    Number(f64), // TODO: Can't put floats as keys in maps, since they don't implement Eq. What to
+    // Note that it doesn't really make sense to compare floats on equality without specifying
-    // do?
+    // 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),
 }
 
@@ -30,7 +39,6 @@ pub enum IndexableValue {
     String(String),
     Int(u64),
     Uuid(Uuid),
-    // TODO: what about null?
 }
 
 impl DbType {