feat: extent based namespaces #3208
Conversation
…/jackgerrits/vowpal_wabbit into jagerrit/extent_based_namespaces
…/jackgerrits/vowpal_wabbit into jagerrit/extent_based_namespaces
jackgerrits
changed the title
vowpalwabbit/feature_group.cc
Outdated
| @@ -140,13 +248,50 @@ bool features::sort(uint64_t parse_mask) | |||
| apply_permutation_in_place(values, dest_index_vec); | |||
There was a problem hiding this comment.
Worried about allocations that can happen in steady state:
- sort operation (seems avoidable)
- flatten_namespace
There was a problem hiding this comment.
Sort is very rarely used, it is only used with limit and sort_features. It is also done only once per example, so in practice this will not matter. Trying to extend what the existing sort implementation was doing to include extents would be quite impractical.
There was a problem hiding this comment.
As discussed the apply function was rolled into one to avoid reallocating the done vec
| @@ -10,6 +10,7 @@ table Namespace { | |||
| name:string; | |||
| hash:uint8; | |||
| features:[Feature]; | |||
| full_hash:uint64; | |||
There was a problem hiding this comment.
a /// comment for this element would be useful.
There was a problem hiding this comment.
Will do. I'd rather call this hash but that is already taken by the other element which is really the index.
There was a problem hiding this comment.
Yeah. I agree with your preference.
There was a problem hiding this comment.
Should I switch them? I am worried that will break things. What do you think?
There was a problem hiding this comment.
Going to leave as is with a comment
| @@ -10,6 +10,7 @@ table Namespace { | |||
| name:string; | |||
| hash:uint8; | |||
There was a problem hiding this comment.
a /// comment for this element would be useful.
Yep. Looks good. Thanks. |
…/jackgerrits/vowpal_wabbit into jagerrit/extent_based_namespaces
No description provided.