mirror of
https://github.com/qdrant/qdrant.git
synced 2026-09-21 13:37:46 -05:00
feat: add a dedicated min operator to score formulas (#10296)
Follow-up to #10287, which added `max`. Expressing a minimum still required spelling out `(a + b - |a - b|) / 2`, the sign flip of the max identity — drop the `neg` and you silently get a maximum instead. It also only works for two operands and mentions each one twice, so the scorer walks every sub-tree twice per candidate point. The pair is what makes clamping expressible: {"max": [0.0, {"min": [1.0, "$score"]}]} `min` mirrors `max` throughout, and both guard helpers introduced in #10287 already took an `operator: &str`, so they are reused unchanged: an empty operand list is rejected at parse time rather than folding to +infinity, and the Edge FFI rejects it at construction time. The result needs no `is_finite` check, since `min` cannot produce a non-finite value from finite inputs. The unindexed-field walker shares one arm for `Max | Min` as the bodies are identical, with a test pinning `min` separately so a later split cannot silently drop it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
0b2b4a4e72
commit
83fe47c90a
@@ -15273,6 +15273,9 @@
|
||||
{
|
||||
"$ref": "#/components/schemas/MaxExpression"
|
||||
},
|
||||
{
|
||||
"$ref": "#/components/schemas/MinExpression"
|
||||
},
|
||||
{
|
||||
"$ref": "#/components/schemas/NegExpression"
|
||||
},
|
||||
@@ -15403,6 +15406,21 @@
|
||||
}
|
||||
}
|
||||
},
|
||||
"MinExpression": {
|
||||
"description": "Smallest of the given expressions. Requires at least one operand.",
|
||||
"type": "object",
|
||||
"required": [
|
||||
"min"
|
||||
],
|
||||
"properties": {
|
||||
"min": {
|
||||
"type": "array",
|
||||
"items": {
|
||||
"$ref": "#/components/schemas/Expression"
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
"NegExpression": {
|
||||
"type": "object",
|
||||
"required": [
|
||||
|
||||
@@ -351,6 +351,7 @@ fn configure_validation(builder: Builder) -> Builder {
|
||||
("MultExpression.mult", ""),
|
||||
("SumExpression.sum", ""),
|
||||
("MaxExpression.max", ""),
|
||||
("MinExpression.min", ""),
|
||||
("DivExpression.left", ""),
|
||||
("DivExpression.right", ""),
|
||||
("PowExpression.base", ""),
|
||||
|
||||
@@ -70,7 +70,8 @@ use crate::grpc::qdrant::{
|
||||
};
|
||||
use crate::grpc::{
|
||||
self, BinaryQuantizationEncoding, BinaryQuantizationQueryEncoding, DecayParamsExpression,
|
||||
DivExpression, GeoDistance, MaxExpression, MultExpression, PowExpression, SumExpression,
|
||||
DivExpression, GeoDistance, MaxExpression, MinExpression, MultExpression, PowExpression,
|
||||
SumExpression,
|
||||
};
|
||||
use crate::rest::models::{CollectionsResponse, ShardKeysResponse, VersionInfo};
|
||||
use crate::rest::schema as rest;
|
||||
@@ -3638,6 +3639,12 @@ fn unparse_expression(
|
||||
.map(|expr| unparse_expression(expr, conditions))
|
||||
.collect(),
|
||||
}),
|
||||
ParsedExpression::Min(exprs) => Variant::Min(MinExpression {
|
||||
min: exprs
|
||||
.into_iter()
|
||||
.map(|expr| unparse_expression(expr, conditions))
|
||||
.collect(),
|
||||
}),
|
||||
ParsedExpression::Neg(expr) => {
|
||||
Variant::Neg(Box::new(unparse_expression(*expr, conditions)))
|
||||
}
|
||||
|
||||
@@ -980,6 +980,8 @@ message Expression {
|
||||
Expression acosh = 20;
|
||||
// Maximum
|
||||
MaxExpression max = 21;
|
||||
// Minimum
|
||||
MinExpression min = 22;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1000,6 +1002,10 @@ message MaxExpression {
|
||||
repeated Expression max = 1;
|
||||
}
|
||||
|
||||
message MinExpression {
|
||||
repeated Expression min = 1;
|
||||
}
|
||||
|
||||
message DivExpression {
|
||||
Expression left = 1;
|
||||
Expression right = 2;
|
||||
|
||||
@@ -6572,7 +6572,7 @@ pub struct Formula {
|
||||
pub struct Expression {
|
||||
#[prost(
|
||||
oneof = "expression::Variant",
|
||||
tags = "1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21"
|
||||
tags = "1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22"
|
||||
)]
|
||||
#[validate(nested)]
|
||||
pub variant: ::core::option::Option<expression::Variant>,
|
||||
@@ -6644,6 +6644,9 @@ pub mod expression {
|
||||
/// Maximum
|
||||
#[prost(message, tag = "21")]
|
||||
Max(super::MaxExpression),
|
||||
/// Minimum
|
||||
#[prost(message, tag = "22")]
|
||||
Min(super::MinExpression),
|
||||
}
|
||||
}
|
||||
#[derive(serde::Serialize)]
|
||||
@@ -6681,6 +6684,14 @@ pub struct MaxExpression {
|
||||
#[derive(validator::Validate)]
|
||||
#[derive(serde::Serialize)]
|
||||
#[derive(Clone, PartialEq, ::prost::Message)]
|
||||
pub struct MinExpression {
|
||||
#[prost(message, repeated, tag = "1")]
|
||||
#[validate(nested)]
|
||||
pub min: ::prost::alloc::vec::Vec<Expression>,
|
||||
}
|
||||
#[derive(validator::Validate)]
|
||||
#[derive(serde::Serialize)]
|
||||
#[derive(Clone, PartialEq, ::prost::Message)]
|
||||
pub struct DivExpression {
|
||||
#[prost(message, optional, boxed, tag = "1")]
|
||||
#[validate(nested)]
|
||||
|
||||
@@ -457,6 +457,7 @@ impl Validate for super::qdrant::expression::Variant {
|
||||
grpc::expression::Variant::Mult(mult_expression) => mult_expression.validate(),
|
||||
grpc::expression::Variant::Sum(sum_expression) => sum_expression.validate(),
|
||||
grpc::expression::Variant::Max(max_expression) => max_expression.validate(),
|
||||
grpc::expression::Variant::Min(min_expression) => min_expression.validate(),
|
||||
grpc::expression::Variant::Div(div_expression) => div_expression.validate(),
|
||||
grpc::expression::Variant::Neg(expression) => expression.validate(),
|
||||
grpc::expression::Variant::Abs(expression) => expression.validate(),
|
||||
|
||||
@@ -938,6 +938,7 @@ pub enum Expression {
|
||||
Mult(MultExpression),
|
||||
Sum(SumExpression),
|
||||
Max(MaxExpression),
|
||||
Min(MinExpression),
|
||||
Neg(NegExpression),
|
||||
Abs(AbsExpression),
|
||||
Div(DivExpression),
|
||||
@@ -994,6 +995,13 @@ pub struct MaxExpression {
|
||||
pub max: Vec<Expression>,
|
||||
}
|
||||
|
||||
/// Smallest of the given expressions. Requires at least one operand.
|
||||
#[derive(Debug, Serialize, Deserialize, JsonSchema, Validate)]
|
||||
pub struct MinExpression {
|
||||
#[validate(nested)]
|
||||
pub min: Vec<Expression>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Serialize, Deserialize, JsonSchema, Validate)]
|
||||
pub struct NegExpression {
|
||||
#[validate(nested)]
|
||||
|
||||
@@ -271,6 +271,7 @@ impl Validate for Expression {
|
||||
Expression::Mult(mult_expression) => mult_expression.validate(),
|
||||
Expression::Sum(sum_expression) => sum_expression.validate(),
|
||||
Expression::Max(max_expression) => max_expression.validate(),
|
||||
Expression::Min(min_expression) => min_expression.validate(),
|
||||
Expression::Neg(neg_expression) => neg_expression.validate(),
|
||||
Expression::Abs(abs_expression) => abs_expression.validate(),
|
||||
Expression::Div(div_expression) => div_expression.validate(),
|
||||
|
||||
@@ -474,7 +474,8 @@ impl<'a> Extractor<'a> {
|
||||
}
|
||||
return;
|
||||
}
|
||||
ExpressionInternal::Max(expression_internals) => {
|
||||
ExpressionInternal::Max(expression_internals)
|
||||
| ExpressionInternal::Min(expression_internals) => {
|
||||
for expr in expression_internals {
|
||||
self.update_from_expression(expr);
|
||||
}
|
||||
@@ -698,6 +699,22 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// The walker shares one arm for `max` and `min`, so this pins down that `min` recurses too
|
||||
/// rather than relying on the shared arm staying shared.
|
||||
#[test]
|
||||
fn min_operands_report_unindexed_payload_fields() {
|
||||
let payload_schema = HashMap::new();
|
||||
|
||||
let mut extractor = Extractor::new(&payload_schema);
|
||||
extractor.update_from_expression(&ExpressionInternal::Min(vec![
|
||||
ExpressionInternal::Variable("$score".to_string()),
|
||||
ExpressionInternal::Variable("popularity".to_string()),
|
||||
]));
|
||||
|
||||
let unindexed: Vec<_> = extractor.unindexed_schema().keys().cloned().collect();
|
||||
assert_eq!(unindexed, vec![JsonPath::new("popularity")]);
|
||||
}
|
||||
|
||||
/// Payload fields referenced inside `max` still need an index, so the walker must recurse
|
||||
/// into its operands the same way it does for `sum` and `mult`. Missing this would silently
|
||||
/// stop suggesting indexes for any field used under a max.
|
||||
|
||||
@@ -221,6 +221,18 @@ impl Expression {
|
||||
})
|
||||
}
|
||||
|
||||
/// Smallest of `operands`. Requires at least one operand, for the same reason as
|
||||
/// [`Expression::max`]: there is no identity element, so an empty list would otherwise
|
||||
/// score every point with +infinity at query time.
|
||||
#[uniffi::constructor]
|
||||
pub fn min(operands: Vec<Arc<Expression>>) -> Result<Arc<Self>> {
|
||||
require_non_empty("min", &operands)?;
|
||||
let children: Vec<&Arc<Expression>> = operands.iter().collect();
|
||||
Self::node(&children, || {
|
||||
ExpressionInternal::Min(operands.iter().map(|e| e.inner.clone()).collect())
|
||||
})
|
||||
}
|
||||
|
||||
/// Negation: `-expression`.
|
||||
#[uniffi::constructor]
|
||||
pub fn negate(expression: Arc<Expression>) -> Result<Arc<Self>> {
|
||||
@@ -357,9 +369,9 @@ impl Expression {
|
||||
// ── Coverage map ────────────────────────────────────────────────────────────
|
||||
|
||||
/// Compile-time map of the engine's formula-expression tree onto the
|
||||
/// Rejects a variadic operator given no operands. `max` has no identity element to fall back on,
|
||||
/// so an empty list would otherwise score every point with -infinity at query time; failing at
|
||||
/// build time points at the mistake instead.
|
||||
/// Rejects a variadic operator given no operands. `max` and `min` have no identity element to
|
||||
/// fall back on, so an empty list would otherwise score every point with -infinity or +infinity
|
||||
/// at query time; failing at build time points at the mistake instead.
|
||||
fn require_non_empty(operator: &str, operands: &[Arc<Expression>]) -> Result<()> {
|
||||
if operands.is_empty() {
|
||||
return Err(EdgeError::invalid_argument(format!(
|
||||
@@ -397,6 +409,8 @@ fn assert_every_expression_is_mapped(e: ExpressionInternal) {
|
||||
ExpressionInternal::Sum(_) => {}
|
||||
// [`Expression::max`]
|
||||
ExpressionInternal::Max(_) => {}
|
||||
// [`Expression::min`]
|
||||
ExpressionInternal::Min(_) => {}
|
||||
// [`Expression::negate`]
|
||||
ExpressionInternal::Neg(_) => {}
|
||||
// [`Expression::div`]
|
||||
|
||||
@@ -2846,6 +2846,93 @@ fn formula_empty_max_is_rejected() {
|
||||
);
|
||||
}
|
||||
|
||||
/// `min` against a dominated constant pins every score to that constant, the mirror of the `max`
|
||||
/// floor test. A negative ceiling also distinguishes `min` from `sum`, which would instead
|
||||
/// subtract from each score.
|
||||
#[test]
|
||||
fn formula_min_clamps_scores_to_a_ceiling() {
|
||||
use qdrant_edge_ffi::{Expression, Prefetch, QueryRequest, ScoringQuery};
|
||||
|
||||
let dir = tempfile::tempdir().expect("tempdir failed");
|
||||
let path = dir.path().to_string_lossy().into_owned();
|
||||
let shard: Arc<EdgeShard> = EdgeShard::load(path, Some(make_config())).expect("load failed");
|
||||
|
||||
let points = vec![
|
||||
Point {
|
||||
id: PointId::NumId { value: 1 },
|
||||
vector: named_vec([0.1, 0.1, 0.1, 0.1]),
|
||||
payload: None,
|
||||
},
|
||||
Point {
|
||||
id: PointId::NumId { value: 2 },
|
||||
vector: named_vec([0.09, 0.09, 0.09, 0.09]),
|
||||
payload: None,
|
||||
},
|
||||
];
|
||||
let op = UpdateOperation::upsert_points(points, None, None).expect("upsert failed");
|
||||
shard.update(op).expect("update failed");
|
||||
|
||||
const CEILING: f32 = -42.0;
|
||||
|
||||
let expression = Expression::min(vec![
|
||||
Expression::variable("$score".to_string()),
|
||||
Expression::constant(CEILING).expect("constant build failed"),
|
||||
])
|
||||
.expect("expression build failed");
|
||||
|
||||
let hits = shard
|
||||
.query(QueryRequest {
|
||||
limit: 2,
|
||||
offset: None,
|
||||
query: Some(ScoringQuery::Formula {
|
||||
expression,
|
||||
defaults: HashMap::new(),
|
||||
}),
|
||||
prefetches: vec![Prefetch {
|
||||
limit: 10,
|
||||
query: Some(ScoringQuery::Vector {
|
||||
query: Query::Nearest {
|
||||
vector: NamedVector::Dense {
|
||||
values: vec![0.1, 0.1, 0.1, 0.1],
|
||||
},
|
||||
using: Some("vec".to_string()),
|
||||
},
|
||||
}),
|
||||
prefetches: vec![],
|
||||
filter: None,
|
||||
score_threshold: None,
|
||||
params: None,
|
||||
}],
|
||||
with_vector: None,
|
||||
with_payload: None,
|
||||
filter: None,
|
||||
score_threshold: None,
|
||||
params: None,
|
||||
})
|
||||
.expect("formula query failed");
|
||||
|
||||
assert_eq!(hits.len(), 2, "both points should be re-scored");
|
||||
for hit in &hits {
|
||||
assert_eq!(
|
||||
hit.score, CEILING,
|
||||
"every score is above the ceiling, so min must return the ceiling itself"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// An empty `min` is rejected for the same reason as an empty `max`: no identity element, so it
|
||||
/// would otherwise score every point with +infinity.
|
||||
#[test]
|
||||
fn formula_empty_min_is_rejected() {
|
||||
use qdrant_edge_ffi::Expression;
|
||||
|
||||
let err = Expression::min(vec![]).expect_err("empty min must be rejected");
|
||||
assert!(
|
||||
matches!(err, EdgeError::InvalidArgument { .. }),
|
||||
"expected InvalidArgument, got {err:?}"
|
||||
);
|
||||
}
|
||||
|
||||
/// Grouped query: one group per category, best hit each.
|
||||
#[test]
|
||||
fn query_groups_returns_one_group_per_key() {
|
||||
|
||||
@@ -2483,6 +2483,11 @@ class Expression(Enum):
|
||||
"""Create a maximum expression. Requires at least one operand."""
|
||||
...
|
||||
|
||||
@staticmethod
|
||||
def Min(exprs: List["Expression"]) -> "Expression":
|
||||
"""Create a minimum expression. Requires at least one operand."""
|
||||
...
|
||||
|
||||
@staticmethod
|
||||
def Neg(expr: "Expression") -> "Expression":
|
||||
"""Create a negation expression."""
|
||||
|
||||
@@ -49,6 +49,10 @@ impl FromPyObject<'_, '_> for PyExpression {
|
||||
ExpressionInternal::Max(PyExpression::peel_vec(exprs))
|
||||
}
|
||||
|
||||
PyExpressionInterface::Min { exprs } => {
|
||||
ExpressionInternal::Min(PyExpression::peel_vec(exprs))
|
||||
}
|
||||
|
||||
PyExpressionInterface::Neg { expr } => ExpressionInternal::Neg(expr.into_box()),
|
||||
|
||||
PyExpressionInterface::Div {
|
||||
@@ -132,6 +136,10 @@ impl<'py> IntoPyObject<'py> for PyExpression {
|
||||
exprs: PyExpression::wrap_vec(exprs),
|
||||
},
|
||||
|
||||
ExpressionInternal::Min(exprs) => PyExpressionInterface::Min {
|
||||
exprs: PyExpression::wrap_vec(exprs),
|
||||
},
|
||||
|
||||
ExpressionInternal::Neg(expr) => PyExpressionInterface::Neg {
|
||||
expr: Boxed::from_box(expr),
|
||||
},
|
||||
@@ -230,6 +238,10 @@ impl Repr for PyExpression {
|
||||
("Max", &[("exprs", &PyExpression::wrap_slice(exprs))])
|
||||
}
|
||||
|
||||
ExpressionInternal::Min(exprs) => {
|
||||
("Min", &[("exprs", &PyExpression::wrap_slice(exprs))])
|
||||
}
|
||||
|
||||
ExpressionInternal::Neg(expr) => ("Neg", &[("expr", PyExpression::wrap_ref(expr))]),
|
||||
|
||||
ExpressionInternal::Div {
|
||||
|
||||
@@ -46,6 +46,10 @@ pub enum PyExpressionInterface {
|
||||
exprs: Vec<PyExpression>,
|
||||
},
|
||||
|
||||
Min {
|
||||
exprs: Vec<PyExpression>,
|
||||
},
|
||||
|
||||
Neg {
|
||||
expr: Boxed<PyExpression>,
|
||||
},
|
||||
@@ -113,6 +117,7 @@ impl Repr for PyExpressionInterface {
|
||||
PyExpressionInterface::Mult { exprs } => ("Mult", &[("exprs", exprs)]),
|
||||
PyExpressionInterface::Sum { exprs } => ("Sum", &[("exprs", exprs)]),
|
||||
PyExpressionInterface::Max { exprs } => ("Max", &[("exprs", exprs)]),
|
||||
PyExpressionInterface::Min { exprs } => ("Min", &[("exprs", exprs)]),
|
||||
PyExpressionInterface::Neg { expr } => ("Neg", &[("expr", expr)]),
|
||||
|
||||
PyExpressionInterface::Div {
|
||||
|
||||
@@ -177,6 +177,14 @@ impl FormulaScorer<'_> {
|
||||
Ok(acc.max(value))
|
||||
})
|
||||
}
|
||||
ParsedExpression::Min(expressions) => {
|
||||
expressions
|
||||
.iter()
|
||||
.try_fold(PreciseScore::INFINITY, |acc, expr| {
|
||||
let value = self.eval_expression(expr, point_id)?;
|
||||
Ok(acc.min(value))
|
||||
})
|
||||
}
|
||||
ParsedExpression::Div {
|
||||
left,
|
||||
right,
|
||||
@@ -465,6 +473,36 @@ mod tests {
|
||||
]), 85.0)]
|
||||
// A single operand is returned as-is
|
||||
#[case(ParsedExpression::Max(vec![ParsedExpression::new_score_id(1)]), 2.0)]
|
||||
// `min` over the same mixed operands picks the smallest instead of the largest
|
||||
#[case(ParsedExpression::Min(vec![
|
||||
ParsedExpression::Constant(PreciseScoreOrdered::from(1.0)),
|
||||
ParsedExpression::new_score_id(0),
|
||||
ParsedExpression::new_payload_id(JsonPath::new(FIELD_NAME)),
|
||||
ParsedExpression::new_condition_id(0),
|
||||
]), 1.0)]
|
||||
#[case(ParsedExpression::Min(vec![ParsedExpression::new_score_id(1)]), 2.0)]
|
||||
// Negative operands: min must not be confused by magnitude
|
||||
#[case(ParsedExpression::Min(vec![
|
||||
ParsedExpression::Constant(PreciseScoreOrdered::from(-10.0)),
|
||||
ParsedExpression::Constant(PreciseScoreOrdered::from(-2.0)),
|
||||
]), -10.0)]
|
||||
// Datetimes evaluate to seconds, so `min` picks the older of two dates.
|
||||
#[case(ParsedExpression::Min(vec![
|
||||
ParsedExpression::Datetime(DatetimeExpression::Constant("2025-03-18".parse().unwrap())),
|
||||
ParsedExpression::Datetime(DatetimeExpression::Constant("2026-01-01".parse().unwrap())),
|
||||
]), "2025-03-18".parse::<DateTimePayloadType>().unwrap().timestamp() as PreciseScore / 1_000_000.0)]
|
||||
// Nested sub-expressions are evaluated before comparing
|
||||
#[case(ParsedExpression::Min(vec![
|
||||
ParsedExpression::Mult(vec![
|
||||
ParsedExpression::Constant(PreciseScoreOrdered::from(3.0)),
|
||||
ParsedExpression::new_score_id(0),
|
||||
]),
|
||||
ParsedExpression::new_score_id(1),
|
||||
]), 2.0)]
|
||||
// max and min of the same single operand agree
|
||||
#[case(ParsedExpression::Min(vec![
|
||||
ParsedExpression::Constant(PreciseScoreOrdered::from(7.5)),
|
||||
]), 7.5)]
|
||||
// Negative operands: max must not be confused by magnitude
|
||||
#[case(ParsedExpression::Max(vec![
|
||||
ParsedExpression::Constant(PreciseScoreOrdered::from(-10.0)),
|
||||
@@ -541,6 +579,17 @@ mod tests {
|
||||
ParsedExpression::Constant(PreciseScoreOrdered::from(5.0)),
|
||||
ParsedExpression::new_log10(ParsedExpression::Constant(PreciseScoreOrdered::from(0.0))),
|
||||
]), 0.0)]
|
||||
// Same for `min`, with the failure on either side of the finite operand.
|
||||
#[should_panic(expected = r#"NonFiniteNumber { expression: "log10(0) = -inf" }"#)]
|
||||
#[case(ParsedExpression::Min(vec![
|
||||
ParsedExpression::new_log10(ParsedExpression::Constant(PreciseScoreOrdered::from(0.0))),
|
||||
ParsedExpression::Constant(PreciseScoreOrdered::from(5.0)),
|
||||
]), 0.0)]
|
||||
#[should_panic(expected = r#"NonFiniteNumber { expression: "log10(0) = -inf" }"#)]
|
||||
#[case(ParsedExpression::Min(vec![
|
||||
ParsedExpression::Constant(PreciseScoreOrdered::from(5.0)),
|
||||
ParsedExpression::new_log10(ParsedExpression::Constant(PreciseScoreOrdered::from(0.0))),
|
||||
]), 0.0)]
|
||||
#[should_panic(expected = r#"NonFiniteNumber { expression: "acosh(0.5) = NaN" }"#)]
|
||||
#[case(
|
||||
ParsedExpression::new_acosh(ParsedExpression::Constant(PreciseScoreOrdered::from(0.5))),
|
||||
|
||||
@@ -66,6 +66,7 @@ pub enum ParsedExpression {
|
||||
Mult(Vec<ParsedExpression>),
|
||||
Sum(Vec<ParsedExpression>),
|
||||
Max(Vec<ParsedExpression>),
|
||||
Min(Vec<ParsedExpression>),
|
||||
Div {
|
||||
left: Box<ParsedExpression>,
|
||||
right: Box<ParsedExpression>,
|
||||
|
||||
@@ -654,6 +654,9 @@ impl From<rest::Expression> for ExpressionInternal {
|
||||
rest::Expression::Max(rest::MaxExpression { max: exprs }) => {
|
||||
ExpressionInternal::Max(exprs.into_iter().map(ExpressionInternal::from).collect())
|
||||
}
|
||||
rest::Expression::Min(rest::MinExpression { min: exprs }) => {
|
||||
ExpressionInternal::Min(exprs.into_iter().map(ExpressionInternal::from).collect())
|
||||
}
|
||||
rest::Expression::Neg(rest::NegExpression { neg: expr }) => {
|
||||
ExpressionInternal::Neg(Box::new(ExpressionInternal::from(*expr)))
|
||||
}
|
||||
@@ -799,6 +802,13 @@ impl TryFrom<grpc::Expression> for ExpressionInternal {
|
||||
.try_collect()?;
|
||||
ExpressionInternal::Max(max)
|
||||
}
|
||||
Variant::Min(grpc::MinExpression { min }) => {
|
||||
let min = min
|
||||
.into_iter()
|
||||
.map(ExpressionInternal::try_from)
|
||||
.try_collect()?;
|
||||
ExpressionInternal::Min(min)
|
||||
}
|
||||
Variant::Div(div) => {
|
||||
let grpc::DivExpression {
|
||||
left,
|
||||
@@ -928,6 +938,25 @@ mod formula_grpc_roundtrip_tests {
|
||||
]));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn min_survives_grpc_roundtrip() {
|
||||
assert_roundtrips(ParsedExpression::Min(vec![
|
||||
ParsedExpression::new_score_id(0),
|
||||
constant(0.5),
|
||||
]));
|
||||
}
|
||||
|
||||
/// `max` and `min` share the same shape, so a copy-paste slip in either unparse arm would
|
||||
/// send one operator's operands out under the other's tag. Nesting them in each other pins
|
||||
/// the two arms apart.
|
||||
#[test]
|
||||
fn max_and_min_do_not_swap_in_grpc_roundtrip() {
|
||||
assert_roundtrips(ParsedExpression::Max(vec![
|
||||
ParsedExpression::Min(vec![constant(1.0), ParsedExpression::new_score_id(0)]),
|
||||
ParsedExpression::Min(vec![constant(2.0), ParsedExpression::new_score_id(1)]),
|
||||
]));
|
||||
}
|
||||
|
||||
/// `max` nested inside other operators must round trip too, not just at the formula root.
|
||||
#[test]
|
||||
fn nested_max_survives_grpc_roundtrip() {
|
||||
|
||||
@@ -61,6 +61,7 @@ pub enum ExpressionInternal {
|
||||
Mult(Vec<ExpressionInternal>),
|
||||
Sum(Vec<ExpressionInternal>),
|
||||
Max(Vec<ExpressionInternal>),
|
||||
Min(Vec<ExpressionInternal>),
|
||||
Neg(Box<ExpressionInternal>),
|
||||
Div {
|
||||
left: Box<ExpressionInternal>,
|
||||
@@ -140,6 +141,9 @@ impl ExpressionInternal {
|
||||
ExpressionInternal::Max(expression_internals) => ParsedExpression::Max(
|
||||
parse_non_empty_operands("max", expression_internals, payload_vars, conditions)?,
|
||||
),
|
||||
ExpressionInternal::Min(expression_internals) => ParsedExpression::Min(
|
||||
parse_non_empty_operands("min", expression_internals, payload_vars, conditions)?,
|
||||
),
|
||||
ExpressionInternal::Neg(expression_internal) => ParsedExpression::new_neg(
|
||||
expression_internal.parse_and_convert(payload_vars, conditions)?,
|
||||
),
|
||||
@@ -204,9 +208,9 @@ impl ExpressionInternal {
|
||||
}
|
||||
|
||||
/// Parses the operands of a variadic operator which has no identity element to fall back on when
|
||||
/// given nothing. `sum` and `mult` can define the empty case as `0` and `1`, but `max` cannot:
|
||||
/// folding over no operands would yield -inf, and score every point with a non-finite value
|
||||
/// instead of reporting the mistake.
|
||||
/// given nothing. `sum` and `mult` can define the empty case as `0` and `1`, but `max` and `min`
|
||||
/// cannot: folding over no operands would yield -inf or +inf, and score every point with a
|
||||
/// non-finite value instead of reporting the mistake.
|
||||
fn parse_non_empty_operands(
|
||||
operator: &str,
|
||||
operands: Vec<ExpressionInternal>,
|
||||
@@ -258,6 +262,49 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn empty_min_is_rejected() {
|
||||
let err = parse(ExpressionInternal::Min(vec![])).unwrap_err();
|
||||
assert!(
|
||||
matches!(err, OperationError::ValidationError { .. }),
|
||||
"expected a validation error, got {err:?}"
|
||||
);
|
||||
let message = err.to_string();
|
||||
assert!(
|
||||
message.contains("min"),
|
||||
"error should name the operator, got {message:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn single_operand_min_is_accepted() {
|
||||
let parsed = parse(ExpressionInternal::Min(vec![ExpressionInternal::Constant(
|
||||
1.0,
|
||||
)]))
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
parsed.formula,
|
||||
ParsedExpression::Min(vec![ParsedExpression::Constant(PreciseScoreOrdered::from(
|
||||
1.0
|
||||
))])
|
||||
);
|
||||
}
|
||||
|
||||
/// Payload variables and conditions nested inside `min` must still be collected, otherwise
|
||||
/// the scorer would have no retriever for them.
|
||||
#[test]
|
||||
fn min_collects_nested_payload_vars() {
|
||||
let parsed = parse(ExpressionInternal::Min(vec![
|
||||
ExpressionInternal::Variable("popularity".to_string()),
|
||||
ExpressionInternal::Variable("$score".to_string()),
|
||||
]))
|
||||
.unwrap();
|
||||
assert_eq!(
|
||||
parsed.payload_vars,
|
||||
HashSet::from([JsonPath::new("popularity")])
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn single_operand_max_is_accepted() {
|
||||
let parsed = parse(ExpressionInternal::Max(vec![ExpressionInternal::Constant(
|
||||
|
||||
@@ -58,6 +58,20 @@ def setup(on_disk_vectors, collection_name):
|
||||
{"max": [{"mult": ["$score", 3.0]}, "price", 0.0]},
|
||||
lambda score, price: max(3.0 * score, price, 0.0),
|
||||
),
|
||||
(
|
||||
{"min": ["$score", "price"]},
|
||||
lambda score, price: min(score, price),
|
||||
),
|
||||
# More than two operands, and nested sub-expressions
|
||||
(
|
||||
{"min": [{"mult": ["$score", 3.0]}, "price", 0.0]},
|
||||
lambda score, price: min(3.0 * score, price, 0.0),
|
||||
),
|
||||
# max and min of the same operands bracket the operands from both sides
|
||||
(
|
||||
{"sum": [{"max": ["$score", "price"]}, {"min": ["$score", "price"]}]},
|
||||
lambda score, price: max(score, price) + min(score, price),
|
||||
),
|
||||
],
|
||||
)
|
||||
def test_formula(collection_name, formula, expecting):
|
||||
@@ -238,3 +252,68 @@ def test_empty_max_is_rejected(collection_name):
|
||||
)
|
||||
assert not response.ok, response.json()
|
||||
assert response.status_code == 400, response.json()
|
||||
|
||||
|
||||
def test_empty_min_is_rejected(collection_name):
|
||||
"""Same as the empty `max` case: no identity element, so it must be rejected rather than
|
||||
silently scoring every point as +infinity."""
|
||||
response = request_with_validation(
|
||||
api="/collections/{collection_name}/points/query",
|
||||
method="POST",
|
||||
path_params={"collection_name": collection_name},
|
||||
body={
|
||||
"prefetch": {"query": 8},
|
||||
"query": {"formula": {"min": []}},
|
||||
},
|
||||
)
|
||||
assert not response.ok, response.json()
|
||||
assert response.status_code == 400, response.json()
|
||||
|
||||
|
||||
def test_min_matches_the_arithmetic_workaround(collection_name):
|
||||
"""The identity for a minimum is (a + b - |a - b|) / 2, the sign flip of the max one.
|
||||
|
||||
Same reasoning as the `max` case: `min` must be a drop-in replacement for formulas
|
||||
already written out by hand.
|
||||
"""
|
||||
point_id = 8
|
||||
boosted = {"mult": [3.0, "$score"]}
|
||||
|
||||
workaround = {
|
||||
"mult": [
|
||||
0.5,
|
||||
{
|
||||
"sum": [
|
||||
boosted,
|
||||
"price",
|
||||
{"neg": {"abs": {"sum": [boosted, {"neg": "price"}]}}},
|
||||
]
|
||||
},
|
||||
]
|
||||
}
|
||||
direct = {"min": [boosted, "price"]}
|
||||
|
||||
def scores_for(formula):
|
||||
response = request_with_validation(
|
||||
api="/collections/{collection_name}/points/query",
|
||||
method="POST",
|
||||
path_params={"collection_name": collection_name},
|
||||
body={
|
||||
"prefetch": {"query": point_id},
|
||||
"query": {"formula": formula, "defaults": {"price": 0.0}},
|
||||
"limit": 10,
|
||||
},
|
||||
)
|
||||
assert response.ok, response.json()
|
||||
return {
|
||||
point["id"]: point["score"] for point in response.json()["result"]["points"]
|
||||
}
|
||||
|
||||
workaround_scores = scores_for(workaround)
|
||||
direct_scores = scores_for(direct)
|
||||
|
||||
assert workaround_scores.keys() == direct_scores.keys()
|
||||
for point_id, expected in workaround_scores.items():
|
||||
assert isclose(direct_scores[point_id], expected, rel_tol=1e-5), (
|
||||
f"point {point_id}: min gave {direct_scores[point_id]}, workaround gave {expected}"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user