handle ip adresses in term aggregation #2319
Conversation
PSeitz
force-pushed
the
term_agg_ip
branch
2 times, most recently
from
564da58
to
7a86ce1
Compare
Stores IpAdresses during the segment term aggregation via u64 representation and convert to u128(IpV6Adress) via downcast when converting to intermediate results. Enable Downcasting on `ColumnValues` Expose u64 variant for u128 encoded data via `open_u64_lenient` method. Remove lifetime in VecColumn, to avoid 'static lifetime requirement coming from downcast trait.
| /// # Notice | ||
| /// In case there are new codecs added, check for usages of `CompactSpaceDecompressorU64` and | ||
| /// also handle the new codecs. | ||
| pub fn open_u128_as_u64(mut bytes: OwnedBytes) -> io::Result<Arc<dyn ColumnValues<u64>>> { |
There was a problem hiding this comment.
can you rename the method? Right now it suggest it is a simple coercion? In reality you are returning the compact space.
Maybe
open_u128_compact_space_as_u64
or something like that
There was a problem hiding this comment.
renamed to open_u128_as_compact_u64
| @@ -63,7 +63,10 @@ impl BlockwiseLinearEstimator { | |||
| if self.block.is_empty() { | |||
| return; | |||
| } | |||
| let line = Line::train(&VecColumn::from(&self.block)); | |||
| let column = VecColumn::from(std::mem::take(&mut self.block)); | |||
There was a problem hiding this comment.
why was this needed? I don't understand.
There was a problem hiding this comment.
Extending the ColumnValues trait with another trait (any trait, even an empty one) requires lifetimes to be 'static. This requirement is no problem when working on the values instead of references, so I changed VecColumn to a owned version.
| .accessor | ||
| .values | ||
| .clone() | ||
| .downcast_arc::<CompactSpaceU64Accessor>() |
There was a problem hiding this comment.
I agree that not doing the segment collection on u128 is the right way here, but I am surprised downcasting logic is required here. Are there really no better way?
There was a problem hiding this comment.
The main problem here is with regards to generics.
We need a codec specific method on a specific trait variant ColumnValues<u64>.
We could return ColumnValuesU64Ext instead of ColumnValues<u64>, which would include the method
trait U64ColumnValuesExt: ColumnValues<u64> {
compact_to_u128(&self, compact: u32) -> u128 {
unimplemented!() // unimplemented for all expect compact space
}
}Having codec specific methods on a common trait is a little bit weird.
Casting is also not great, but having the mechanism may also allow some performance gains by enabling inlining in some cases.
In both cases we will need to know the underlying codec before calling the codec specific method.
* handle ip adresses in term aggregation Stores IpAdresses during the segment term aggregation via u64 representation and convert to u128(IpV6Adress) via downcast when converting to intermediate results. Enable Downcasting on `ColumnValues` Expose u64 variant for u128 encoded data via `open_u64_lenient` method. Remove lifetime in VecColumn, to avoid 'static lifetime requirement coming from downcast trait. * rename method
Stores IpAdresses during the segment term aggregation via u64 representation
and convert to u128(IpV6Adress) via downcast when converting to intermediate results.
Enable Downcasting on
ColumnValuesExpose u64 variant for u128 encoded data via
open_u64_lenientmethod.Remove lifetime in VecColumn, to avoid 'static lifetime requirement coming
from downcast trait.
#1689