-
-
Notifications
You must be signed in to change notification settings - Fork 679
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
add OptionalColumn #1685
add OptionalColumn #1685
Conversation
76d7cc9
to
107c090
Compare
107c090
to
6591886
Compare
6591886
to
191fe51
Compare
Codecov Report
@@ Coverage Diff @@
## main #1685 +/- ##
==========================================
- Coverage 94.04% 93.89% -0.16%
==========================================
Files 256 257 +1
Lines 49169 49391 +222
==========================================
+ Hits 46243 46374 +131
- Misses 2926 3017 +91
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
/// For instance, the min value does not take in account of possible | ||
/// deleted document. All values are however guaranteed to be higher than | ||
/// `.min_value()`. | ||
fn min_value(&self) -> Option<T>; |
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.
I wonder if we should make it T
and force this trait to have at least one value?
same for max_value.
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.
Then we would need to return Option<dyn OptionalColumn>
. It's a valid alternative, but the handling may be more complex
|
||
/// Temporary wrapper to migrate to optional column | ||
pub(crate) struct ToOptionalColumn<T> { | ||
column: Arc<dyn Column<T>>, |
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.
this implies two dispatches.
I think we can:
- make it a generic
- add a trait boundary to Column:
Column: Clone
- add a to_optional to
Column
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.
I started with a generic, but did run into compiler issues with the to_full
method. It requires additional trait bounds Clone + 'static
. I chose the easy solution since it's only temporary.
impl<C: Column + Clone + 'static> OptionalColumn for ToOptionalColumn<C>{
fn to_full(&self) -> Arc<dyn Column>{
Arc::new(self.underlying.clone())
}
}
Co-authored-by: Paul Masurel <paul@quickwit.io>
Perf regression doesn't seem to bad for the
Option<T>
handling on aggregations#1678