-
Notifications
You must be signed in to change notification settings - Fork 100
feat: add OpenSearch distance operations and selectors #1335
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
base: master
Are you sure you want to change the base?
Conversation
jgaleotti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add test cases for the classes OpenSearchHeuristicsCalculator, ParameterExtractor and OpenSearchQueryHelper
| import org.evomaster.client.java.controller.opensearch.selectors.MatchSelector; | ||
| import org.evomaster.client.java.controller.opensearch.selectors.QuerySelector; | ||
|
|
||
| public class OpenSearchQueryParser { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need tests for OpenSearchQueryParser to check that the JSON is correctly parsed into its corresponding OpenSearch operation.
| /** | ||
| * Utility class to extract common parameters and reduce code duplication in selectors. | ||
| */ | ||
| public class ParameterExtractor { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add test cases for this class
| /** | ||
| * Extracts the case_insensitive parameter from a term query object. | ||
| */ | ||
| public static Boolean extractCaseInsensitive(Object query, String structure) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add test cases for this class
arcuri82
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for late check, but was on vacation most of last week
|
|
||
| if (operation instanceof MatchOperation) { | ||
| return calculateDistanceForMatch((MatchOperation) operation, doc); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't these be a chain of if/else, with a final having a warn log if no match was present? (eg to see if we miss any case).
| // Not enough matches - distance based on how many more matches needed | ||
| int shortfall = minimumRequired - matchCount; | ||
| // Return a distance that reflects both the shortfall and the quality of non-matching terms | ||
| return shortfall * 10.0 + (totalDistance / expectedTerms.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if expectedTerms.size() is 0?
|
|
||
| try { | ||
| // Create pattern with case insensitive flag if needed | ||
| java.util.regex.Pattern pattern; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the regex in OpenSearch having the same syntax of Java regex? clarify here in some comments, with possible some links
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenSearch uses Lucene regular expression syntax, which is similar but NOT identical to Java regex.
https://opensearch.org/docs/latest/query-dsl/term/regexp/
https://lucene.apache.org/core/8_0_0/core/org/apache/lucene/util/automaton/RegExp.html
@jgaleotti Do you consider adding Java Regexp for this heuristic calculation as acceptable?
|
|
||
| // Helper methods for advanced operations | ||
|
|
||
| private int calculateEditDistance(String s1, String s2, boolean allowTranspositions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
geneneric helper functions on strings should not be here, but rather in client-java/distance-heuristics/src/main/java/org/evomaster/client/java/distance/heuristics/DistanceHelper.java
| // } | ||
| @GetMapping("gte/{gte}") | ||
| public List<Age> findGteAge(@PathVariable("gte") Integer gte) throws IOException { | ||
| return ageRepo.findGteAge(gte); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all these endpoints should return 404 if no data is found
| Solution<RestIndividual> solution = initAndRun(args); | ||
|
|
||
| // assertHasAtLeastOne(solution, HttpVerb.GET, 404, "/students/{q}", null); | ||
| assertHasAtLeastOne(solution, HttpVerb.GET, 200, "/students/{lastName}", null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add assertions for both 404 and 200 cases
| // Term selector tests | ||
| @GetMapping("category/{category}") | ||
| public List<Product> findByCategory(@PathVariable String category) throws IOException { | ||
| return productRepository.findByCategory(category); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do return 404 if nothing is found in these endpoints, as it will simplify writing assertions/checks in the tests
|
|
||
| Solution<RestIndividual> solution = initAndRun(args); | ||
|
|
||
| assertHasAtLeastOne(solution, HttpVerb.GET, 200, "/queries/category/{category}", null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add assertions on 404
| } | ||
|
|
||
| @Test | ||
| public void testRangeQueries() throws Throwable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these E2E repeated with same configurations? can have single tests with all the assertions.
otherwise, if you only want to test specific endpoints, you have to pass args parameters to specify them, eg, with --endpointPrefix
| @@ -0,0 +1,21 @@ | |||
| package com.opensearch.queries; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
folder e2e-tests does not exist any more (we did a major refactoring of folder structures recently)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing that.
|
@arcuri82 is there any known issue with the Is there any way to prevent those errors? |
|
@mmiguerodriguez hi. there is no known current issue about |
|
I'm not completely sure what was the issue, perhaps different Java version used by Maven and IntelliJ builds... I'll be reviewing why there's some errors on the build/tests. I've already made some of the adjustements. |
|
@mmiguerodriguez ah, yes. we moved to JDK 17 recently... and might move to 21 in the near future |
|
We'll need to adjust |
|
@mmiguerodriguez good point! i ll do now |
No description provided.