[score boosting] Only accept finite numbers#6092
Conversation
📝 WalkthroughWalkthroughThis pull request modifies error handling and expression parsing for mathematical operations across several modules. Key changes include updating the Possibly related PRs
Suggested Reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (13)
🪧 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: 0
🧹 Nitpick comments (1)
lib/segment/src/index/query_optimization/rescore_formula/formula_scorer.rs (1)
182-184: Consider adding finite check for exponential function.While the exponential function typically produces finite results for most reasonable inputs, it can overflow for large input values. For consistency with other mathematical operations, consider adding a similar finite check for
expoperations.let value = self.eval_expression(parsed_expression, point_id)?; - Ok(value.exp()) + let exp_value = value.exp(); + if exp_value.is_finite() { + Ok(exp_value) + } else { + Err(OperationError::NonFiniteNumber { + expression: format!("exp({value})"), + }) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/api/src/grpc/conversions.rs(1 hunks)lib/collection/src/operations/types.rs(1 hunks)lib/segment/src/common/operation_error.rs(1 hunks)lib/segment/src/index/query_optimization/rescore_formula/formula_scorer.rs(4 hunks)lib/segment/src/index/query_optimization/rescore_formula/parsed_formula.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/api/src/grpc/conversions.rs
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: test-snapshot-operations-s3-minio
- GitHub Check: test-shard-snapshot-api-s3-minio
- GitHub Check: test-low-resources
- GitHub Check: Basic TLS/HTTPS tests
- GitHub Check: test (macos-latest)
- GitHub Check: test-consensus-compose
- GitHub Check: test (windows-latest)
- GitHub Check: test-consensus
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test
- GitHub Check: test
🔇 Additional comments (13)
lib/segment/src/common/operation_error.rs (1)
70-71: Good addition of a dedicated error type for non-finite numbers.This new error variant will appropriately handle cases where mathematical operations produce non-finite results (infinity, NaN), making it easier to debug formula evaluation issues and provide better error messages to users.
lib/collection/src/operations/types.rs (1)
1247-1247: Appropriate error mapping for non-finite numbers.This change correctly maps the new
OperationError::NonFiniteNumbertoCollectionError::BadInput, which is the right categorization since non-finite results are typically caused by invalid input data rather than system failures.lib/segment/src/index/query_optimization/rescore_formula/formula_scorer.rs (7)
147-149: Improved error handling for division by zero.Instead of silently returning a default value for division by zero, the code now properly returns an error with a descriptive message when no default value is provided. This makes the behavior more explicit and helps identify formula issues.
161-167: Better error handling for square root of negative numbers.This change ensures that non-finite results from square root operations (like trying to take the square root of a negative number) are properly handled with a descriptive error rather than returning NaN or infinity.
173-179: Enhanced error detection for power operations.Power operations can produce non-finite results in various scenarios (negative base with non-integer exponent, large values causing overflow, etc.). This change ensures these cases are properly detected and reported with clear error messages.
187-194: Proper handling of logarithm domain errors.Logarithm operations on non-positive numbers now properly return an error with a descriptive message instead of producing Infinity or NaN values that could silently propagate through calculations.
198-205: Consistent error handling for natural logarithm.Similar to the base-10 logarithm, the natural logarithm function now properly handles non-finite results with clear error messages.
308-310: Updated test case for division with optional default.The test case correctly reflects the API change where
by_zero_defaultis now anOption<ScoreType>instead ofScoreType.
320-336: Good test coverage for non-finite error cases.The added test cases ensure that non-finite error conditions are properly tested. This is essential for validating the new error handling behavior for mathematical operations.
lib/segment/src/index/query_optimization/rescore_formula/parsed_formula.rs (4)
41-41: Enhanced the Div expression to support optional default values.Changing
by_zero_defaultfromScoreTypetoOption<ScoreType>provides more flexibility in handling division by zero cases. It allows the caller to either specify a default value or request an error to be returned.
88-88: Updated constructor to use optional default value.The
new_divconstructor method has been correctly updated to directly use theby_zero_defaultparameter without applying a default value.
100-118: Good addition of constructor methods for mathematical operations.Adding constructor methods for power, square root, logarithm (base 10), and natural logarithm operations improves code readability and consistency. These methods encapsulate the boilerplate of wrapping expressions in
Box.
81-89:Details
✅ Verification successful
Verify existing callers of
new_divare updated.Make sure that all existing callers of
new_divhave been updated to handle the change fromScoreTypetoOption<ScoreType>for theby_zero_defaultparameter. This API change could cause compilation errors if not all callers are updated.
🏁 Script executed:
#!/bin/bash # Search for all calls to new_div that might need updating rg -p "new_div\(" --type rustLength of output: 391
Caller Updates Verified – No Further Changes Needed
After running a repository-wide search, we confirmed that all existing callers of
ParsedExpression::new_divhave been updated to match the new signature using anOption<ScoreType>. The search revealed calls in the following files:
- lib/collection/src/operations/universal_query/formula.rs (line 89)
- lib/segment/src/index/query_optimization/rescore_formula/formula_scorer.rs (lines 308 and 326)
Please review these locations to ensure the optional parameter is correctly passed (e.g., using
Some(...)orNoneas appropriate). No additional modifications appear necessary.
* return error for non-finite numbers in expression * no internal default for division by zero * add tests * review suggestions
2 participants
We decided to only handle finite numbers in the formula evaluation.
With these changes, we now return a new bad request error when the evaluation leads to infinite or NaN.
Test cases were added to assert the errors