Conversation
Merging this PR will degrade performance by 10.2%
Performance Changes
Comparing Footnotes
|
There was a problem hiding this comment.
Note that once we get ScalarValue::Array (#6717), this will be a lot less ugly.
| // also be nullable (null vectors produce null norms). | ||
| let storage = extension_storage(&vector_array)?; | ||
| let l2_norm_expr = | ||
| Expression::try_new(ScalarFn::new(L2Norm, EmptyOptions).erased(), [root()])?; |
There was a problem hiding this comment.
Short cut would be L2Norm.new_expr(...)
There was a problem hiding this comment.
Even then, we should have a nicer way of doing this!!
| let norms_array = norms_prim.clone().into_array(); | ||
|
|
||
| // Extract flat elements from the (always non-nullable) storage for normalization. | ||
| let flat = extract_flat_elements(&storage, list_size, ctx)?; |
There was a problem hiding this comment.
It's a helper function I use for all of the other tensor types to make sure we can deal with ConstantArray correctly:
/// Extracts the flat primitive elements from a tensor storage array (FixedSizeList).
///
/// When the input is a [`ConstantArray`] (e.g., a literal query vector), only a single row is
/// materialized to avoid expanding it to the full column length.
| let storage = extension_storage(&vector_array)?; | ||
| let l2_norm_expr = | ||
| Expression::try_new(ScalarFn::new(L2Norm, EmptyOptions).erased(), [root()])?; | ||
| let norms_prim: PrimitiveArray = vector_array.apply(&l2_norm_expr)?.execute(ctx)?; |
There was a problem hiding this comment.
You said vector array can be nullable, but you don't check validity when iterating below
| // Extract flat elements from the (always non-nullable) storage for normalization. | ||
| let flat = extract_flat_elements(&storage, list_size, ctx)?; | ||
|
|
||
| match_each_float_ptype!(flat.ptype(), |T| { |
There was a problem hiding this comment.
This all seems a bit convoluted? Can you just run-end encode the norms by the vector lengths, then use a divide function?
There was a problem hiding this comment.
So this is what the decompress path would look like (the compress path would be similar):
// We need to multiply each vector element with its respective norm. We do not have any kind
// of "broadcast" expression to each of the `FixedSizeList` elements, so we can mimic this
// by multiplying the normalized vector array with a `RunEnd(Sequence)` array.
let base: PValue = list_size.into();
let multiplier: PValue = base;
let ends_ptype = base.ptype();
let ends_nullability = Nullability::NonNullable;
let ends =
SequenceArray::try_new(base, multiplier, ends_ptype, ends_nullability, num_vectors)?;
let runend = RunEndArray::try_new(ends.into_array(), self.norms.clone())?;
let storage = extension_storage(&self.vector_array)?;
let fsl: FixedSizeListArray = storage.execute(ctx)?;
let elements = fsl.elements();
debug_assert!(elements.dtype().is_primitive());
let decompress = elements.binary(runend.into_array(), Operator::Mul)?;
let denormalized_elements: PrimitiveArray = decompress.execute(ctx)?;
// SAFETY: TODO
let fsl = unsafe {
FixedSizeListArray::new_unchecked(
denormalized_elements.into_array(),
list_size,
self.validity()?,
num_vectors,
)
};
Ok(ExtensionArray::new(ext.clone(), fsl.into_array()).into_array())
}I don't think that this is actually less convoluted? But maybe we do this regardless because the optimizer might be able to do something
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
|
ok just realized that a bunch of stuff was wrong because I changed from non-nullable to nullable halfway through, will fix everything |
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Summary
Tracking Issue: #6865
Related PR: #7018
Adds a new encoding specific to the vector extension type.
Note that we cannot actually utilize this by adding it to the compressor until we make it pluggable (see #7018). When that does eventually land, we can simply create a
NormVectorSchemethat uses theNormVectorArray.Implementation
We currently do not have a good way of broadcasting a multiplication onto a
FixedSizeList, so this implementation hand rolls the norm multiplication. Additionally, we still do not have #6717 so thescalar_atimplementation is also slow.API Changes
Adds a new encoding type
NormVectorTesting
Some simple tests including roundtrips.