Skip to content

Commit

Permalink
Merge 'Fix table with single column PRIMARY KEY to not create extra b…
Browse files Browse the repository at this point in the history
…tree' from Krishna Vishal

The error is due to comparing the PRIMARY KEY's name to INTEGER when in
it was all in lowercase. This was causing `needs_auto_index` to be set
to `true`.
After the fix:
```
/limbo /tmp/sc2-limbo.db
Limbo v0.0.13
Enter ".help" for usage hints.
limbo> CREATE TABLE temp (t1 integer, primary key (t1));

hexdump -s 28 -n 4 /tmp/sc2-limbo.db
000001c 0000 0200 -- matches SQLite
0000020
```
Closes #824

Reviewed-by: Jussi Saurio <[email protected]>

Closes #830
  • Loading branch information
penberg committed Jan 31, 2025
2 parents c05b81f + 8b2393f commit d8a9c57
Showing 1 changed file with 33 additions and 11 deletions.
44 changes: 33 additions & 11 deletions core/translate/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,11 @@ fn emit_schema_entry(
});
}

struct PrimaryKeyColumnInfo<'a> {
name: &'a String,
is_descending: bool,
}

/// Check if an automatic PRIMARY KEY index is required for the table.
/// If so, create a register for the index root page and return it.
///
Expand Down Expand Up @@ -291,10 +296,13 @@ fn check_automatic_pk_index_required(
columns: pk_cols, ..
} = &constraint.constraint
{
let primary_key_column_results: Vec<Result<&String>> = pk_cols
let primary_key_column_results: Vec<Result<PrimaryKeyColumnInfo>> = pk_cols
.iter()
.map(|col| match &col.expr {
ast::Expr::Id(name) => Ok(&name.0),
ast::Expr::Id(name) => 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(),
Expand All @@ -306,7 +314,9 @@ fn check_automatic_pk_index_required(
if let Err(e) = result {
bail_parse_error!("{}", e);
}
let column_name = result?;
let pk_info = result?;

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);
Expand All @@ -323,8 +333,11 @@ fn check_automatic_pk_index_required(
let column_def = column_def.unwrap();
let typename =
column_def.col_type.as_ref().map(|t| t.name.as_str());
primary_key_definition =
Some(PrimaryKeyDefinitionType::Simple { typename });
let is_descending = pk_info.is_descending;
primary_key_definition = Some(PrimaryKeyDefinitionType::Simple {
typename,
is_descending,
});
}
}
}
Expand All @@ -342,8 +355,10 @@ fn check_automatic_pk_index_required(
bail_parse_error!("table {} has more than one primary key", tbl_name);
}
let typename = col_def.col_type.as_ref().map(|t| t.name.as_str());
primary_key_definition =
Some(PrimaryKeyDefinitionType::Simple { typename });
primary_key_definition = Some(PrimaryKeyDefinitionType::Simple {
typename,
is_descending: false,
});
}
}
}
Expand All @@ -356,9 +371,13 @@ fn check_automatic_pk_index_required(
// Check if we need an automatic index
let needs_auto_index = if let Some(primary_key_definition) = &primary_key_definition {
match primary_key_definition {
PrimaryKeyDefinitionType::Simple { typename } => {
let is_integer = typename.is_some() && typename.unwrap() == "INTEGER";
!is_integer
PrimaryKeyDefinitionType::Simple {
typename,
is_descending,
} => {
let is_integer =
typename.is_some() && typename.unwrap().to_uppercase() == "INTEGER";
!is_integer || *is_descending
}
PrimaryKeyDefinitionType::Composite => true,
}
Expand Down Expand Up @@ -529,7 +548,10 @@ fn translate_create_table(
}

enum PrimaryKeyDefinitionType<'a> {
Simple { typename: Option<&'a str> },
Simple {
typename: Option<&'a str>,
is_descending: bool,
},
Composite,
}

Expand Down

0 comments on commit d8a9c57

Please sign in to comment.