Repository: tursodatabase/limbo (rust)

Issue:
Create table panic bugs related to constraints
#1494 was merged but contains some bugs:

```sql
limbo> create table t4(a, unique(b));

thread 'main' panicked at core/schema.rs:988:29:
Column b is in index sqlite_autoindex_t4_1 but not found in table t4
```

```sql
limbo> create table t2(a,b, unique(a,b), primary key(a,b));

thread 'main' panicked at core/schema.rs:1012:13:
number of auto_indices in schema should be same number of indices calculated
```

```sql
limbo> create table t4(a,b, primary key(a,b), unique(a));

thread 'main' panicked at core/schema.rs:981:71:
number of auto_indices in schema should be same number of indices calculated
```

Pull request:
Autoindex fix
Closes #1508 . There were two small issues to fix:

1. We were not checking in the IndexMap of columns, if the unique column name is declared in the composite declaration exists in the IndexMap. This solved the first this statement `create table t4(a, unique(b));`.
2. The second thing was that we forgot to add the column_name to the HashSet of columns. 
```rust
Some(PrimaryKeyDefinitionType::Simple { column, .. }) => {
      let mut columns = HashSet::new();
      columns.insert(std::mem::take(column));
      // Have to also insert the current column_name we are iterating over in primary_key_column_results
      columns.insert(column_name.clone()); <-- Fix here
      primary_key_definition =
           Some(PrimaryKeyDefinitionType::Composite { columns });
      }
```
The rest of the modifications are just some small simplifications for readability and avoiding some clones


Patch:
diff --git a/core/translate/schema.rs b/core/translate/schema.rs
index 4942a956..1f95f89d 100644
--- a/core/translate/schema.rs
+++ b/core/translate/schema.rs
@@ -249,6 +249,7 @@ pub fn emit_schema_entry(
     });
 }
 
+#[derive(Debug)]
 struct PrimaryKeyColumnInfo<'a> {
     name: &'a String,
     is_descending: bool,
@@ -288,36 +289,41 @@ fn check_automatic_pk_index_required(
                         if primary_key_definition.is_some() {
                             bail_parse_error!("table {} has more than one primary key", tbl_name);
                         }
-                        let primary_key_column_results: Vec<Result<PrimaryKeyColumnInfo>> = pk_cols
+                        let primary_key_column_results = pk_cols
                             .iter()
                             .map(|col| match &col.expr {
-                                ast::Expr::Id(name) => Ok(PrimaryKeyColumnInfo {
-                                    name: &name.0,
-                                    is_descending: matches!(col.order, Some(ast::SortOrder::Desc)),
-                                }),
+                                ast::Expr::Id(name) => {
+                                    if !columns.iter().any(|(k, _)| k.0 == name.0) {
+                                        bail_parse_error!("No such column: {}", name.0);
+                                    }
+                                    Ok(PrimaryKeyColumnInfo {
+                                        name: &name.0,
+                                        is_descending: matches!(
+                                            col.order,
+                                            Some(ast::SortOrder::Desc)
+                                        ),
+                                    })
+                                }
                                 _ => Err(LimboError::ParseError(
                                     "expressions prohibited in PRIMARY KEY and UNIQUE constraints"
                                         .to_string(),
                                 )),
                             })
-                            .collect();
-
-                        for result in primary_key_column_results {
-                            if let Err(e) = result {
-                                bail_parse_error!("{}", e);
-                            }
-                            let pk_info = result?;
+                            .collect::<Result<Vec<_>>>()?;
 
+                        for pk_info in primary_key_column_results {
                             let column_name = pk_info.name;
-                            let column_def = columns.get(&ast::Name(column_name.clone()));
-                            if column_def.is_none() {
-                                bail_parse_error!("No such column: {}", column_name);
-                            }
+                            let (_, column_def) = columns
+                                .iter()
+                                .find(|(k, _)| k.0 == *column_name)
+                                .expect("primary key column should be in Create Body columns");
 
                             match &mut primary_key_definition {
                                 Some(PrimaryKeyDefinitionType::Simple { column, .. }) => {
                                     let mut columns = HashSet::new();
-                                    columns.insert(column.clone());
+                                    columns.insert(std::mem::take(column));
+                                    // Have to also insert the current column_name we are iterating over in primary_key_column_results
+                                    columns.insert(column_name.clone());
                                     primary_key_definition =
                                         Some(PrimaryKeyDefinitionType::Composite { columns });
                                 }
@@ -325,7 +331,6 @@ fn check_automatic_pk_index_required(
                                     columns.insert(column_name.clone());
                                 }
                                 None => {
-                                    let column_def: &ast::ColumnDefinition = column_def.unwrap();
                                     let typename =
                                         column_def.col_type.as_ref().map(|t| t.name.as_str());
                                     let is_descending = pk_info.is_descending;
@@ -339,24 +344,28 @@ fn check_automatic_pk_index_required(
                             }
                         }
                     } else if let ast::TableConstraint::Unique {
-                        columns,
+                        columns: unique_columns,
                         conflict_clause,
                     } = &constraint.constraint
                     {
                         if conflict_clause.is_some() {
                             unimplemented!("ON CONFLICT not implemented");
                         }
-                        let col_names: HashSet<String> = columns
+
+                        let col_names = unique_columns
                             .iter()
                             .map(|column| match &column.expr {
                                 limbo_sqlite3_parser::ast::Expr::Id(id) => {
-                                    crate::util::normalize_ident(&id.0)
+                                    if !columns.iter().any(|(k, _)| k.0 == id.0) {
+                                        bail_parse_error!("No such column: {}", id.0);
+                                    }
+                                    Ok(crate::util::normalize_ident(&id.0))
                                 }
                                 _ => {
                                     todo!("Unsupported unique expression");
                                 }
                             })
-                            .collect();
+                            .collect::<Result<HashSet<String>>>()?;
                         unique_sets.push(col_names);
                     }
                 }
@@ -436,6 +445,7 @@ fn check_automatic_pk_index_required(
     }
 }
 
+#[derive(Debug)]
 enum PrimaryKeyDefinitionType<'a> {
     Simple {
         column: String,


Test patch:
diff --git a/testing/all.test b/testing/all.test
index b0ea003b..1f9ab805 100755
--- a/testing/all.test
+++ b/testing/all.test
@@ -33,3 +33,4 @@ source $testdir/default_value.test
 source $testdir/boolean.test
 source $testdir/literal.test
 source $testdir/null.test
+source $testdir/create_table.test
diff --git a/testing/create_table.test b/testing/create_table.test
new file mode 100755
index 00000000..858914f3
--- /dev/null
+++ b/testing/create_table.test
@@ -0,0 +1,16 @@
+#!/usr/bin/env tclsh
+
+set testdir [file dirname $argv0]
+source $testdir/tester.tcl
+
+do_execsql_test_in_memory_any_error create_table_one_unique_set {
+    CREATE TABLE t4(a, unique(b));
+}
+
+do_execsql_test_on_specific_db {:memory:} create_table_same_uniques_and_primary_keys {
+    CREATE TABLE t2(a,b, unique(a,b), primary key(a,b));
+} {}
+
+do_execsql_test_on_specific_db {:memory:} create_table_unique_contained_in_primary_keys {
+    CREATE TABLE t4(a,b, primary key(a,b), unique(a));
+} {}