OpenSearch Distance Calculation Feature #1301
Conversation
…distance-calculation
…distance-calculation
| @@ -323,6 +327,17 @@ public final void initMongoHandler() { | |||
| } | |||
| } | |||
|
|
|||
| public final void initOpenSearchHandler() { | |||
There was a problem hiding this comment.
it is fine in this PR. but, as this code seems very similar to what done for Mongo, maybe @jgaleotti you can refactor it to avoid duplication?
@mmiguerodriguez maybe leave a // TODO comment stating this code likely needs refactoring with the Mongo one
There was a problem hiding this comment.
please leave a TODO and when the Redis and the OpenSearch implementations are done I will refactor them out.
There was a problem hiding this comment.
Will add TODO comment regarding refactors
| try { | ||
| openSearchHandler.handle(it); | ||
| } catch (Exception e){ | ||
| SimpleLogger.error("FAILED TO HANDLE OPENSEARCH COMMAND"); |
There was a problem hiding this comment.
add info on the error in this log, eg, + e.getMssage()
|
|
||
| public final double distance; | ||
|
|
||
| public final int numberOfEvaluatedDocuments; |
There was a problem hiding this comment.
those fields should be private, as anyway you are providing public getters
| return calculateDistanceForEquals((TermOperation<?>) operation, doc); | ||
| } | ||
|
|
||
| return 0; |
There was a problem hiding this comment.
is it correct to return 0 here? if so, it is not so obvious. add some /* */ comments to clarify it
There was a problem hiding this comment.
I guess this should be Double.MAX_VALUE as this is the case where it is not supported, right?
There was a problem hiding this comment.
Yes, will modify this.
| private final String fieldName; | ||
| private final V value; | ||
|
|
||
| ComparisonOperation(String fieldName, V value) { |
There was a problem hiding this comment.
default-scope in Java is very tricky, and possibly counter-intuitive. as such, I usually avoid it. consider if a protected makes sense here
| Object kind = value.getClass().getMethod("_kind").invoke(value); | ||
| String kindName = (String) kind.getClass().getMethod("name").invoke(kind); | ||
|
|
||
| switch (kindName) { |
There was a problem hiding this comment.
what about Integer and Short?
There was a problem hiding this comment.
I used OpenSearch FieldValue as reference. Will review if more are required.
|
|
||
| public class OpenSearchCommand implements Serializable { | ||
| /** | ||
| * Name of the index that the operation was applied to | ||
| */ | ||
| private final Object index; | ||
| private final List<String> index; |
There was a problem hiding this comment.
update docs based on new type. ie, how can a list of strings be a name?
There was a problem hiding this comment.
Yeah, this might be confusing, will review how the correct naming should be.
I think that for an initial implementation we should consider only simple Strings, without lists.
| } else if (result instanceof String) { | ||
| return Collections.singletonList((String) result); | ||
| } else { | ||
| return Collections.singletonList(result.toString()); |
There was a problem hiding this comment.
is this last case expected in normal behaviour? or is it just a safety check to not crash EM? in this latter case, might want to log such event (so we can see when/if it happens, and fix it if needed)
There was a problem hiding this comment.
I made it this way to consider all cases, but the normal behavior would be for the index to be a single String.
| return ageRepo.findByAge(age); | ||
| } | ||
|
|
||
| // @GetMapping("gte/{gte}") |
There was a problem hiding this comment.
is this code no longer needed? or to be put back in future? either remove it, or add a TODO comment if you ll need it in future
There was a problem hiding this comment.
Will be put back when testing other types of queries (eg: Range). Will add TODO comment.
|
|
||
| Solution<RestIndividual> solution = initAndRun(args); | ||
|
|
||
| // assertHasAtLeastOne(solution, HttpVerb.GET, 404, "/age/{q}", null); |
There was a problem hiding this comment.
why no assertions? if the techniques work, when you should be able to cover the different endpoints you have designed. if not yet, and tehcniques needs more fixing, it is ok to have a PR now, but: (1) add the needed assertions on the different endpoints; (2) mark the test with @Disabled; (3) leave a TODO comment stating this is failing and need more work
This branch extends EvoMaster with distance calculation support for OpenSearch queries.
Features
Distance Data Structures
OpenSearchHandler
Testing