Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved vector range argument parsing #445

Merged
merged 4 commits into from
Apr 26, 2024

Conversation

abbottdev
Copy link
Contributor

@abbottdev abbottdev commented Apr 25, 2024

When trying to use a VectorRange query using a non-constant value, an exception is thrown:

Exception has occurred: CLR/System.InvalidCastException
Exception thrown: 'System.InvalidCastException' in Redis.OM.dll: 'Unable to cast object of type 'System.Linq.Expressions.UnaryExpression' to type 'System.Linq.Expressions.ConstantExpression'.'
   at Redis.OM.Common.ExpressionParserUtilities.TranslateVectorRange(MethodCallExpression exp, List`1 parameters)
   at Redis.OM.Common.ExpressionParserUtilities.TranslateMethodExpressions(MethodCallExpression exp, List`1 parameters)
   at Redis.OM.Common.ExpressionTranslator.BuildQueryFromExpression(Expression exp, List`1 parameters)
   at Redis.OM.Common.ExpressionTranslator.TranslateWhereMethod(MethodCallExpression expression, List`1 parameters)
   at Redis.OM.Common.ExpressionTranslator.BuildQueryFromExpression(Expression expression, Type type, Expression mainBooleanExpression, Type rootType)
   at Redis.OM.Searching.RedisCollectionEnumerator`1..ctor(Expression exp, IRedisConnection connection, Int32 chunkSize, RedisCollectionStateManager stateManager, Expression`1 booleanExpression, Boolean saveState, Type rootType, Type type)
   at Redis.OM.Searching.RedisCollection`1.GetAsyncEnumerator(CancellationToken cancellationToken)

Support therefore needs to be added to evaluate the expression rather than being hardcoded to a constant expression.

NB The Git LFS thing is still a rake that I stood on so I've included a note about running fetch-models until it's sorted :)

Have run the docker instance locally and have passing tests:

Passed!  - Failed:     0, Passed:     5, Skipped:     0, Total:     5, Duration: 2 s - Redis.OM.Vectorizer.Tests.dll (net7.0)
Passed!  - Failed:     0, Passed:   504, Skipped:    11, Total:   515, Duration: 7 s - Redis.OM.Unit.Tests.dll (net6.0)
Passed!  - Failed:     0, Passed:   504, Skipped:    11, Total:   515, Duration: 7 s - Redis.OM.Unit.Tests.dll (net7.0)

Look forward to feedback!

Copy link
Member

@slorello89 slorello89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 thank you for adding this

@slorello89 slorello89 merged commit 34fb4ca into redis:main Apr 26, 2024
1 check passed
@abbottdev abbottdev deleted the feature/expression-parsing branch April 26, 2024 22:38
slorello89 pushed a commit that referenced this pull request Jun 6, 2024
* Better argument parsing for vector range expressions

* Update contributors

* Simplify expression cases, include test coverage for method expressions.

* Reuse GetOperand for evaluating values

---------

Co-authored-by: James Abbott <james.abbott@crispthinking.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants