Add data structures for variably-sized fixed columns #1542
Conversation
georgwiese
force-pushed
the
variable-fixed-column-size
branch
from
77c7792
to
d9061d2
Compare
georgwiese
changed the title
| serde_cbor::from_reader(file).unwrap() | ||
| impl<T: DeserializeOwned + Serialize> ReadWrite for T { | ||
| fn read(file: &mut impl Read) -> Self { | ||
| serde_cbor::from_reader(file).unwrap() |
There was a problem hiding this comment.
unrelated to this PR but we should probably use BufRead here
There was a problem hiding this comment.
Do you mean just changing to file: &mut impl BufRead? What would be the difference? It is just a trait that adds more functions, which apparently serde_cbor::from_reader() does not use?
number/src/lib.rs
Outdated
|
|
||
| #[derive(Serialize, Deserialize)] | ||
| /// Like Columns, but each column can exist in multiple sizes | ||
| pub struct VariablySizedColumns<F> { |
There was a problem hiding this comment.
I don't think this should be inside number
There was a problem hiding this comment.
Hmm, do you have a suggestion?
Of the common dependencies of backend and executor, for example, it seems like the best fit:
Also, this crate handles the serialization (which arguably it shouldn't), so I think it kind of fits.
There was a problem hiding this comment.
It's much more than just "number". Either we create a new crate or rename this one.
There was a problem hiding this comment.
why not put it inside the executor? Isn't it one of its main outputs?
There was a problem hiding this comment.
True, changed it!
number/src/lib.rs
Outdated
| @@ -31,6 +33,60 @@ pub fn log2_exact(n: BigUint) -> Option<usize> { | |||
| .filter(|zeros| n == (BigUint::from(1u32) << zeros)) | |||
| } | |||
|
|
|||
| pub type Columns<F> = Vec<(String, Vec<F>)>; | |||
There was a problem hiding this comment.
I think this type name over-simplifies (i.e. there should be no typedef ;) ). I would like to know that this is a vector and that it contains the name as a string.
number/src/lib.rs
Outdated
| /// Like Columns, but each column can exist in multiple sizes | ||
| pub struct VariablySizedColumns<F> { | ||
| /// Maps each column name to a (size -> values) map | ||
| columns: Vec<(String, BTreeMap<usize, Vec<F>>)>, |
There was a problem hiding this comment.
I don't think we gain much hiding variably sized columns in a collection of them.
Wouldn't it be better to use a type for ONE column that has a variable size? Then there could be free functions that reduce a collection of those to a unique size.
The len() function is very misleading for example. Does it return the number of different sizes? If this is just a Vec<String, VariableLengthColumn<T>> it's much clearer.
|
Should we really store multiple copies of the values? Wouldn't we want to return a slice instead (plus manage metadata about which slice lengths are "legal")? |
The problem with that is that we sometimes have columns that are different in the last row, for example here. |
georgwiese
force-pushed
the
variable-fixed-column-size
branch
from
0c43fdf
to
1de8252
Compare
number/src/lib.rs
Outdated
| } | ||
| } | ||
|
|
||
| pub fn get_only_size<F>( |
There was a problem hiding this comment.
Maybe get_uniquely_sized? With this name, it could mean that it only returns the size.
georgwiese
force-pushed
the
variable-fixed-column-size
branch
from
31c1e54
to
a2a5968
Compare
georgwiese
force-pushed
the
variable-fixed-column-size
branch
from
a2a5968
to
b90a386
Compare
This PR adds
number::VariablySizedColumns, which can store several sizes of the same column. Currently, we always just have one size, but as part of #1496, we can relax that.