Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(torii-sqlite): primitive upgrades towards bigger types #2931

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+73 −1
Open

feat(torii-sqlite): primitive upgrades towards bigger types #2931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6fb9f39
refactor(torii): primitive upgrades towards bigger types
Larkooo Jan 20, 2025
0792db5
fmt
Larkooo Jan 20, 2025
f2affb6
fix can upgrade to and column mod
Larkooo Jan 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions crates/dojo/types/src/primitive.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,42 @@ impl Primitive {
}
}
}

pub fn can_upgrade_to(&self, other: &Primitive) -> bool {
use Primitive::*;
match (self, other) {
// Boolean (no upgrades)
(Bool(_), Bool(_)) => true,

// Unsigned integer upgrades
(U8(_), U16(_) | U32(_) | USize(_) | U64(_) | U128(_) | Felt252(_)) => true,
(U16(_), U32(_) | USize(_) | U64(_) | U128(_) | Felt252(_)) => true,
(U32(_), USize(_) | U64(_) | U128(_) | Felt252(_)) => true,
(USize(_), U32(_) | U64(_) | U128(_) | Felt252(_)) => true,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Verify USize upgrade path to prevent data loss.

The USize type can upgrade to U32 which might cause data loss on 64-bit systems where usize is 64 bits.

Apply this diff to fix the upgrade path:

-            (USize(_), U32(_) | U64(_) | U128(_) | Felt252(_)) => true,
+            (USize(_), U64(_) | U128(_) | Felt252(_)) => true,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
(USize(_), U32(_) | U64(_) | U128(_) | Felt252(_)) => true,
(USize(_), U64(_) | U128(_) | Felt252(_)) => true,

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a comment here #2925 (comment), I think having usize non-changeable would make more sense at the cairo level (for Torii, it's way less relevant, but it is to match the cairo rules).

(U64(_), U128(_) | Felt252(_)) => true,
(U128(_), Felt252(_)) => true,

// U256 can only upgrade to itself
(U256(_), U256(_)) => true,

// Signed integer upgrades
(I8(_), I16(_) | I32(_) | I64(_) | I128(_) | Felt252(_)) => true,
(I16(_), I32(_) | I64(_) | I128(_) | Felt252(_)) => true,
(I32(_), I64(_) | I128(_) | Felt252(_)) => true,
(I64(_), I128(_) | Felt252(_)) => true,
(I128(_), Felt252(_)) => true,

// Felt252 and address types
(Felt252(_), ClassHash(_) | ContractAddress(_)) => true,
(ClassHash(_), Felt252(_) | ContractAddress(_)) => true,
(ContractAddress(_), Felt252(_) | ClassHash(_)) => true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're missing EthAddress, not sure at which point it's relevant for now and may be supported later.


// Same type is considered an upgrade (no-op)
(a, b) if std::mem::discriminant(a) == std::mem::discriminant(b) => true,

_ => false,
}
}
}

#[cfg(test)]
Expand Down
38 changes: 37 additions & 1 deletion crates/torii/sqlite/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -847,6 +847,21 @@ fn add_columns_recursive(
));
};

let modify_column =
|alter_table_queries: &mut Vec<String>, name: &str, sql_type: &str, sql_value: &str| {
// SQLite doesn't support ALTER COLUMN directly, so we need to:
// 1. Create a new temporary column
// 2. Copy the data
// 3. Drop the old column
// 4. Rename the temporary column
alter_table_queries
.push(format!("ALTER TABLE [{table_id}] ADD COLUMN [tmp_{name}] {sql_type}"));
alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}"));
alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]"));
alter_table_queries
.push(format!("ALTER TABLE [{table_id}] RENAME COLUMN [tmp_{name}] TO [{name}]"));
};
Comment on lines +850 to +863
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure atomic operations with transaction handling.

The column modification process involves multiple SQL operations that should be atomic to prevent data corruption if any step fails.

Wrap the operations in a transaction by adding these queries at the start and end:

+            alter_table_queries.push("BEGIN TRANSACTION;".to_string());
             alter_table_queries
                 .push(format!("ALTER TABLE [{table_id}] ADD COLUMN [tmp_{name}] {sql_type}"));
             alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}"));
             alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]"));
             alter_table_queries
                 .push(format!("ALTER TABLE [{table_id}] RENAME COLUMN [tmp_{name}] TO [{name}]"));
+            alter_table_queries.push("COMMIT;".to_string());

Also, add error handling:

+            alter_table_queries.push("ROLLBACK;".to_string());
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let modify_column =
|alter_table_queries: &mut Vec<String>, name: &str, sql_type: &str, sql_value: &str| {
// SQLite doesn't support ALTER COLUMN directly, so we need to:
// 1. Create a new temporary column
// 2. Copy the data
// 3. Drop the old column
// 4. Rename the temporary column
alter_table_queries
.push(format!("ALTER TABLE [{table_id}] ADD COLUMN [tmp_{name}] {sql_type}"));
alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}"));
alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]"));
alter_table_queries
.push(format!("ALTER TABLE [{table_id}] RENAME COLUMN [tmp_{name}] TO [{name}]"));
};
let modify_column =
|alter_table_queries: &mut Vec<String>, name: &str, sql_type: &str, sql_value: &str| {
// SQLite doesn't support ALTER COLUMN directly, so we need to:
// 1. Create a new temporary column
// 2. Copy the data
// 3. Drop the old column
// 4. Rename the temporary column
alter_table_queries.push("BEGIN TRANSACTION;".to_string());
alter_table_queries
.push(format!("ALTER TABLE [{table_id}] ADD COLUMN [tmp_{name}] {sql_type}"));
alter_table_queries.push(format!("UPDATE [{table_id}] SET [tmp_{name}] = {sql_value}"));
alter_table_queries.push(format!("ALTER TABLE [{table_id}] DROP COLUMN [{name}]"));
alter_table_queries
.push(format!("ALTER TABLE [{table_id}] RENAME COLUMN [tmp_{name}] TO [{name}]"));
alter_table_queries.push("COMMIT;".to_string());
alter_table_queries.push("ROLLBACK;".to_string());
};


match ty {
Ty::Struct(s) => {
for member in &s.children {
Expand Down Expand Up @@ -940,7 +955,28 @@ fn add_columns_recursive(
let column_name =
if column_prefix.is_empty() { "value".to_string() } else { column_prefix };

add_column(&column_name, p.to_sql_type().as_ref());
if let Some(upgrade_diff) = upgrade_diff {
if let Some(old_primitive) = upgrade_diff.as_primitive() {
if old_primitive.can_upgrade_to(p) {
// Modify existing column to new type
modify_column(
alter_table_queries,
&column_name,
p.to_sql_type().as_ref(),
p.to_sql_value().as_ref(),
);
} else {
return Err(anyhow::anyhow!(
"Invalid primitive type upgrade from {:?} to {:?}",
old_primitive,
p
));
}
}
} else {
// New column
add_column(&column_name, p.to_sql_type().as_ref());
}
}
}

Expand Down
Loading