Conversation
📝 WalkthroughWalkthroughThe pull request extends the mathematical expression capabilities across multiple modules. It introduces new variants for operations such as absolute value, square root, power, exponential, base‑10 logarithm, and natural logarithm. These changes span the gRPC protocol definitions, REST schemas, internal expression representations, and evaluation logic. The extensions include new fields in protobuf messages, additional enum variants and structs in REST, and enhanced conversion and evaluation functions within the internal query and scoring systems. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as GRPC/REST API
participant Converter as Expression Converter
participant QueryEngine as Internal Query System
participant Evaluator as FormulaScorer
Client->>API: Send expression request (e.g., pow, sqrt)
API->>Converter: Convert external expression to internal representation
Converter->>QueryEngine: Map to ExpressionInternal variants (including new operations)
QueryEngine->>Evaluator: Evaluate expression (e.g., compute pow, sqrt, etc.)
Evaluator-->>QueryEngine: Return computed result
QueryEngine-->>API: Forward evaluation result
API-->>Client: Return final result
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
lib/api/src/grpc/proto/points.proto (2)
590-595: Add documentation comments for new mathematical operations.Consider adding descriptive comments for each new operation to improve API documentation:
- Expression abs = 9; // Absolute value - Expression sqrt = 10; // Square root - PowExpression pow = 11; // Power - Expression exp = 12; // Exponential - Expression log10 = 13; // Logarithm - Expression ln = 14; // Natural logarithm + Expression abs = 9; // Returns the absolute value of the expression + Expression sqrt = 10; // Returns the square root of the expression + PowExpression pow = 11; // Returns the base expression raised to the power of the exponent expression + Expression exp = 12; // Returns e raised to the power of the expression + Expression log10 = 13; // Returns the base-10 logarithm of the expression + Expression ln = 14; // Returns the natural logarithm (base e) of the expression
618-621: Add documentation comments for PowExpression message.Consider adding descriptive comments for the PowExpression message and its fields:
+// Message for representing a power operation with a base and exponent message PowExpression { - Expression base = 1; - Expression exponent = 2; + Expression base = 1; // The base expression to be raised to a power + Expression exponent = 2; // The exponent expression }lib/api/src/grpc/qdrant.rs (2)
5223-5240: LGTM! Consider enhancing documentation with domain constraints.The new mathematical operations are well-organized and properly tagged. However, consider adding domain constraints to the documentation:
Sqrt: Input must be non-negativeLog10andLn: Input must be positiveApply this diff to enhance the documentation:
/// Absolute value #[prost(message, tag = "9")] Abs(::prost::alloc::boxed::Box<super::Expression>), - /// Square root + /// Square root (input must be non-negative) #[prost(message, tag = "10")] Sqrt(::prost::alloc::boxed::Box<super::Expression>), /// Power #[prost(message, tag = "11")] Pow(::prost::alloc::boxed::Box<super::PowExpression>), /// Exponential #[prost(message, tag = "12")] Exp(::prost::alloc::boxed::Box<super::Expression>), - /// Logarithm + /// Logarithm base 10 (input must be positive) #[prost(message, tag = "13")] Log10(::prost::alloc::boxed::Box<super::Expression>), - /// Natural logarithm + /// Natural logarithm (input must be positive) #[prost(message, tag = "14")] Ln(::prost::alloc::boxed::Box<super::Expression>),
5280-5285: LGTM! Consider adding documentation for PowExpression.The struct is well-designed with appropriate traits and field types. However, it would benefit from documentation explaining its purpose and any domain constraints.
Apply this diff to add documentation:
+/// Represents a power operation with a base and an exponent. +/// Both base and exponent can be complex expressions. +/// Note: Special cases like 0^0 or negative base with fractional exponent should be handled during evaluation. pub struct PowExpression { #[prost(message, optional, boxed, tag = "1")] pub base: ::core::option::Option<::prost::alloc::boxed::Box<Expression>>, #[prost(message, optional, boxed, tag = "2")] pub exponent: ::core::option::Option<::prost::alloc::boxed::Box<Expression>>, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/api/src/grpc/proto/points.proto(2 hunks)lib/api/src/grpc/qdrant.rs(3 hunks)lib/api/src/rest/schema.rs(3 hunks)lib/collection/src/operations/universal_query/formula.rs(2 hunks)lib/collection/src/operations/universal_query/shard_query.rs(2 hunks)lib/segment/src/index/query_optimization/rescore_formula/formula_scorer.rs(1 hunks)lib/segment/src/index/query_optimization/rescore_formula/parsed_formula.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-low-resources
- GitHub Check: test-consistency
- GitHub Check: test-consensus
- GitHub Check: test
- GitHub Check: test (macos-latest)
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test-consensus-compose
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (8)
lib/api/src/grpc/qdrant.rs (1)
5188-5191: LGTM! Tags are properly ordered for the new mathematical operations.The tags sequence is continuous and properly ordered to accommodate the new mathematical operations while maintaining backward compatibility.
lib/segment/src/index/query_optimization/rescore_formula/parsed_formula.rs (1)
44-52: LGTM! The new mathematical operations are well-structured.The new variants in
ParsedExpressionare correctly implemented:
- Each variant appropriately uses
Box<ParsedExpression>for recursive types- The
Powvariant correctly captures both base and exponent expressionslib/collection/src/operations/universal_query/shard_query.rs (2)
249-267: LGTM! The conversion logic is correctly implemented.The
parse_and_convertmethod properly handles the new mathematical operations, maintaining consistency with the existing pattern.
538-563: LGTM! The gRPC conversion is well-implemented.The
TryFrom<grpc::Expression>implementation:
- Properly validates required fields for the Pow variant
- Maintains consistent error handling
- Follows the established pattern for other variants
lib/collection/src/operations/universal_query/formula.rs (2)
29-37: LGTM! The internal expression types are well-defined.The new variants in
ExpressionInternalare correctly implemented:
- Each variant appropriately uses
Box<ExpressionInternal>for recursive types- The
Powvariant correctly captures both base and exponent expressions
86-104: LGTM! The REST conversion is properly implemented.The
From<rest::Expression>implementation correctly handles all new mathematical operations, maintaining consistency with the existing pattern.lib/api/src/rest/schema.rs (2)
644-650: LGTM! The REST API schema is well-structured.The new variants in the
Expressionenum are correctly defined and ordered.
669-715: LGTM! The expression structs are properly implemented.The new expression structs:
- Follow the established pattern
- Use appropriate derive macros
- Properly use Box for recursive types
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/grpc/docs.md (1)
215-215: Markdown List IndentationThe static analysis hint indicates that an unordered list item at line 215 is indented with 4 spaces instead of the expected 2. Please adjust the indentation to conform with the markdown style guidelines. For example, if the current line is similar to:
- - [SomeItem](#some-anchor) + - [SomeItem](#some-anchor)this would resolve the issue.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
215-215: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/grpc/docs.md(3 hunks)docs/redoc/master/openapi.json(3 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/grpc/docs.md
215-215: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-low-resources
- GitHub Check: test-consistency
- GitHub Check: test (macos-latest)
- GitHub Check: test-consensus-compose
- GitHub Check: test (windows-latest)
- GitHub Check: test-consensus
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (15)
docs/grpc/docs.md (2)
2703-2708: New Mathematical Operations in the Expression SectionThe addition of new rows for
abs,sqrt,pow,exp,log10, andlnis clear and consistent with the existing style. In particular, using[PowExpression]as the link for thepowoperation sets it apart, which is appropriate given that its definition is expanded in its own section. Please verify that the anchor names (e.g.#qdrant-Expressionand#qdrant-PowExpression) match exactly the corresponding anchors in the document.
3715-3724: PowExpression Section DocumentationThe newly added "PowExpression" section is well-structured, with a concise header and a table clearly defining the two fields:
baseandexponent. This nicely complements the newpowentry in the Expression table.docs/redoc/master/openapi.json (13)
14294-14296: New mathematical operation: Absolute valueAdded new
AbsExpressionschema for computing absolute values in expressions.
14300-14302: New mathematical operation: Square rootAdded new
SqrtExpressionschema for computing square roots in expressions.
14303-14305: New mathematical operation: PowerAdded new
PowExpressionschema for computing power operations in expressions.
14306-14308: New mathematical operation: ExponentialAdded new
ExpExpressionschema for computing exponential (e^x) operations in expressions.
14309-14311: New mathematical operation: Base-10 logarithmAdded new
Log10Expressionschema for computing base-10 logarithms in expressions.
14312-14314: New mathematical operation: Natural logarithmAdded new
LnExpressionschema for computing natural logarithms in expressions.
14359-14369: Schema definition for absolute value operationThe
AbsExpressionschema is properly defined with:
- Required field "abs" of type Expression
- Follows consistent schema pattern with other mathematical operations
14401-14411: Schema definition for square root operationThe
SqrtExpressionschema is properly defined with:
- Required field "sqrt" of type Expression
- Follows consistent schema pattern with other mathematical operations
14412-14437: Schema definition for power operationThe
PowExpressionandPowParamsschemas are properly defined with:
- Required fields "base" and "exponent" of type Expression
- Follows consistent schema pattern with other mathematical operations
- Appropriate parameter structure for binary operation
14438-14448: Schema definition for exponential operationThe
ExpExpressionschema is properly defined with:
- Required field "exp" of type Expression
- Follows consistent schema pattern with other mathematical operations
14449-14459: Schema definition for base-10 logarithm operationThe
Log10Expressionschema is properly defined with:
- Required field "log10" of type Expression
- Follows consistent schema pattern with other mathematical operations
14460-14470: Schema definition for natural logarithm operationThe
LnExpressionschema is properly defined with:
- Required field "ln" of type Expression
- Follows consistent schema pattern with other mathematical operations
14294-14470:Details
❓ Verification inconclusive
Verify mathematical operations are properly integrated
The new mathematical operations are properly integrated into the
Expressiontype union, allowing them to be used in formulas and other expressions throughout the API.
🏁 Script executed:
#!/bin/bash # Verify that all new expression types are properly referenced in the Expression schema ast-grep --pattern $'anyOf": [ $$$ { "$ref": "#/components/schemas/AbsExpression" }, $$$ { "$ref": "#/components/schemas/SqrtExpression" }, $$$ { "$ref": "#/components/schemas/PowExpression" }, $$$ { "$ref": "#/components/schemas/ExpExpression" }, $$$ { "$ref": "#/components/schemas/Log10Expression" }, $$$ { "$ref": "#/components/schemas/LnExpression" }, $$$'Length of output: 424
Math Operations Integration Needs Manual Verification
The
openapi.jsonsnippet shows all the new mathematical operation references (AbsExpression, DivExpression, SqrtExpression, PowExpression, ExpExpression, Log10Expression, LnExpression, and GeoDistance) visibly present. However, our initial automated AST search did not return any output. This suggests that the precise AST pattern we used might not match the actual JSON structure—even though the snippet appears correct.
- Action: Please manually verify that the Expression type union (or equivalent structure) has been updated to include these operations as intended. This includes confirming that the new math operation references are part of the union used throughout the API.
| // Undefined: Send this point to the bottom of results | ||
| Ok(f32::NEG_INFINITY) |
There was a problem hiding this comment.
Does this work for euclid as well, because we invert later?
There was a problem hiding this comment.
All score boosting will have Order::LargerIsBetter, this will be added to documentation. For the case of euclidean distance, users will have to negate the scores, to be explicit.
* sqrt, pow, exp, log, ln, abs * upd openapi and grpc * handle undefined math as negative infinity
2 participants
Adds the following operations to expression: