Skip to content
28 changes: 23 additions & 5 deletions crates/storage/src/merkle/column.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ use alloc::{
Eq,
Hash,
enum_iterator::Sequence,
strum_macros::EnumCount,
strum_macros::IntoStaticStr,
)]
pub enum MerkleizedColumn<TC> {
Expand All @@ -31,6 +30,16 @@ pub enum MerkleizedColumn<TC> {
MerkleMetadataColumn,
}

impl<TC> strum::EnumCount for MerkleizedColumn<TC>
where
TC: strum::EnumCount + AsU32,
{
/// The total count of variants in the enum.
/// Since we have two columns for each table column and one for the merkle data,
/// we have to multiply the count of the table columns by 2 and add one for the merkle metadata.
const COUNT: usize = TC::COUNT * 2 + 1;
}

Comment thread
netrome marked this conversation as resolved.
/// The trait to convert the column to the `u32`.
pub trait AsU32 {
/// Returns the `u32` representation of the `Column`.
Expand All @@ -42,21 +51,30 @@ where
TC: strum::EnumCount + AsU32,
{
/// The total count of variants in the enum.
/// Since we have two columns for each table column and one for the merkle data,
/// we have to multiply the count of the table columns by 2 and add one for the merkle metadata.
pub const COUNT: usize = TC::COUNT * 2 + 1;
pub const COUNT: usize = <Self as strum::EnumCount>::COUNT;

/// The start of the merkle data columns.
pub const MERKLE_DATA_COLUMNS_START: u32 = u16::MAX as u32;

/// The merkle metadata column
pub const MERKLE_METADATA_COLUMN: u32 = {
assert!(Self::COUNT <= u32::MAX as usize);
// this is fine, because we already performed an assertion above
// see https://github.com/rust-lang/rust-clippy/issues/9613
#[allow(clippy::cast_possible_truncation)]
let column_index = (Self::COUNT as u32).saturating_sub(1);
assert!(column_index > 0);
column_index
};
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If you do it like this, it means that column id depends on the Self::COUNT. So if you add a new column, it will override the metadata column. Metadata column always should be stable regardless of the number of columns in the type.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

addressed in aa438a2


/// Returns the `u32` representation of the `Column`.
pub fn as_u32(&self) -> u32 {
match self {
Self::TableColumn(column) => column.as_u32(),
Self::MerkleDataColumn(column) => {
Self::MERKLE_DATA_COLUMNS_START.wrapping_add(column.as_u32())
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we need to fix this one as well. Seems like the in-memory database implementation assumes these values are contiguous.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nop, we don't need to fix these. the in memory db creates the columns based on the total count. anything in between is fine

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But u16::MAX isn't in-between though, or am I missing something here? Self::MERKLE_DATA_COLUMNS_START is a constant set to u16::MAX from what I see.

Self::MerkleMetadataColumn => u32::MAX,
Self::MerkleMetadataColumn => Self::MERKLE_METADATA_COLUMN,
}
}
}
Expand Down
Loading