-
-
Notifications
You must be signed in to change notification settings - Fork 671
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.
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
collect columns for merge #1812
Conversation
5a8108a
to
b00869c
Compare
columnar/src/column_index/mod.rs
Outdated
let end = multivalued_index.get_val(row_id + 1); | ||
start..end | ||
unimplemented!() | ||
// let start = multivalued_index.get_val(row_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why unimplemented!? Did you spot a bug?
/// Returns a list of `DynamicColumn` which are all of the same numerical type | ||
fn cast_to_common_numerical_column(columns: &[DynamicColumn]) -> Vec<DynamicColumn> { | ||
assert!(columns.iter().all(|column| column.is_numerical())); | ||
let coerce_to_i64: Vec<_> = columns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can probably do something a little bit nicer here, and be more coupled with the writer logic?
fn acceptable_numerical_type(numerical_types: &[NumericalType]) -> NumericalType {
let mut compatible_numerical_types = CompatibleNumericalTypes::default();
for &typ in numerical_types {
compatible_numerical_types.accept(typ);
}
numerical_types.to_numeric_type()
}
struct DynamicColummn {
fn column_type(&self) -> ColumnType { ... }
fn coerce(&self, column_type: ColumnType) -> Option<DynamicColumn> { ... }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the type is not enough, we need to know if the a i64 column could be a u64 column (!contains negative values)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah good point
columnar/src/dynamic_column.rs
Outdated
} | ||
} | ||
|
||
pub struct MapI64ToF64; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we could replace those three thing by the following?
I don;'t know any trait boudnary for as.
If you want to stick to as
, can you make those private to this module?
Technically these mapping are not monotonic.
pub struct Coerce<FromTyp, ToTyp>;
impl<FromTyp, ToTyp> StrictlyMonotonicFn<FromTyp, ToTyp> for MapI64ToF64
where FromTyp: TryFrom<ToTyp>, ToTyp: TryFrom<FromTyp>
{
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe with num traits, but the use case is too small
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments inline
Codecov Report
@@ Coverage Diff @@
## main #1812 +/- ##
==========================================
- Coverage 94.14% 94.12% -0.02%
==========================================
Files 274 274
Lines 51992 51992
==========================================
- Hits 48946 48939 -7
- Misses 3046 3053 +7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
* collect columns for merge * return column_type from, fix visibility * fix Co-authored-by: Paul Masurel <paul@quickwit.io>
No description provided.