Constant comparison and byte_length OnPair kernels#8371
Conversation
Signed-off-by: Mikhail Kot <mikhail@spiraldb.com>
myrrc
force-pushed
the
myrrc/onpair-compare
branch
from
f834ee0 to
0fe87ba
Compare
Merging this PR will not alter performance
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decompress_rd[f64, (10000, 0.0)] |
111.3 µs | 138.3 µs | -19.53% |
| ❌ | Simulation | decompress_rd[f64, (10000, 0.1)] |
111.3 µs | 138.1 µs | -19.41% |
| ❌ | Simulation | decompress_rd[f64, (10000, 0.01)] |
111 µs | 137.7 µs | -19.4% |
| ❌ | Simulation | decompress_rd[f32, (10000, 0.1)] |
80.7 µs | 89.7 µs | -10.05% |
| ⚡ | WallTime | cuda/bitpacked_u8/unpack/3bw[100M] |
351.3 µs | 300.6 µs | +16.85% |
| ⚡ | Simulation | decompress_rd[f64, (100000, 0.0)] |
980.4 µs | 845.5 µs | +15.96% |
| ⚡ | Simulation | encode_varbin[(1000, 4)] |
160.2 µs | 142.5 µs | +12.48% |
| ⚡ | Simulation | encode_varbin[(1000, 8)] |
160.9 µs | 143.1 µs | +12.45% |
| ⚡ | Simulation | encode_varbin[(1000, 32)] |
166.3 µs | 148.1 µs | +12.28% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing myrrc/onpair-compare (0fe87ba) with develop (0dd63f0)
| impl CompareKernel for OnPair { | ||
| fn compare( | ||
| lhs: ArrayView<'_, Self>, | ||
| rhs: &ArrayRef, | ||
| operator: CompareOperator, | ||
| ctx: &mut ExecutionCtx, | ||
| ) -> VortexResult<Option<ArrayRef>> { | ||
| let Some(constant) = rhs.as_constant() else { | ||
| return Ok(None); | ||
| }; | ||
| let is_empty = match constant.dtype() { | ||
| DType::Utf8(_) => constant.as_utf8().is_empty(), | ||
| DType::Binary(_) => constant.as_binary().is_empty(), | ||
| _ => return Ok(None), | ||
| }; | ||
| if is_empty != Some(true) { | ||
| return Ok(None); | ||
| } | ||
|
|
||
| let lengths = lhs.uncompressed_lengths(); | ||
| let buffer = match operator { | ||
| // every value is greater than an empty string | ||
| CompareOperator::Gte => BitBuffer::new_set(lhs.len()), | ||
| // no value is less than an empty string | ||
| CompareOperator::Lt => BitBuffer::new_unset(lhs.len()), | ||
| _ => lengths | ||
| .binary( | ||
| ConstantArray::new(Scalar::zero_value(lengths.dtype()), lengths.len()) | ||
| .into_array(), | ||
| operator.into(), | ||
| )? | ||
| .execute(ctx)?, | ||
| }; | ||
| Ok(Some( |
There was a problem hiding this comment.
lets make a trait over the two string arrays we can use to impl this?
There was a problem hiding this comment.
I started with a trait but it's a worse approach, because the only thing we can re-implement is comparison with an empty constant and then the diff is twice as large
I don't think over-generalisation is a good thing here.
| let lengths = lhs.uncompressed_lengths(); | ||
| let buffer = match operator { | ||
| // every value is greater than an empty string | ||
| CompareOperator::Gte => BitBuffer::new_set(lhs.len()), |
There was a problem hiding this comment.
todo once we can have a constant with validity
There was a problem hiding this comment.
Isn’t Mask scalar function that?
3 participants
This PR adds 2 kernels for OnPair:
This PR also adds validity() for FSST's kernel for byte_length so that
evaluating validity wouldn't mean decompressing whole FSST array.