CLIENT-4614 index on sets - #31
Conversation
BrianNichols
left a comment
BrianNichols
left a comment
There was a problem hiding this comment.
These changes looks reasonable to me. Since it involves AEL code, I will defer to Tim's review.
| String querySet = Optional.ofNullable(indexContext) | ||
| .map(IndexContext::getQuerySet) | ||
| .orElse(null); |
There was a problem hiding this comment.
This is an anti-pattern and a poor use of Optional. Any time you do .orElse(null) it's a strong signal that Optional is being force into a role it's not designed for.
Why not just use
String querySet = indexContext == null ? null : indexContext.getQuerySet();
? It saves a couple of memory allocations and is clearer code.
There was a problem hiding this comment.
I agree on both places. Its just copy paste from existing code. I will clean it up.
| .orElse(null); | ||
| Map<String, List<Index>> indexesMap = buildIndexesMap( | ||
| Optional.ofNullable(indexContext).map(IndexContext::getIndexes).orElse(null), namespace); | ||
| Optional.ofNullable(indexContext).map(IndexContext::getIndexes).orElse(null), |
There was a problem hiding this comment.
As above. (I know it's existing code, but let's fix it up. The next line too.)
| public ParseResult process(String namespace, String querySet, Session session) { | ||
| return new ParseResult(null, exp); |
There was a problem hiding this comment.
I would put a comment in saying the querySet has been deliberately ignored to show that it has been considered (low importance)
|
|
||
| @Override | ||
| public ParseResult process(String namespace, Session session) { | ||
| public ParseResult process(String namespace, String querySet, Session session) { |
There was a problem hiding this comment.
I would put a comment in saying the querySet has been deliberately ignored to show that it has been considered (low importance)
There was a problem hiding this comment.
I have added comments. I think the class structure can be improved by some refactors, so we wouldnt have to use methods that pass unused params. Will do that separately from this change.
3 participants
Applies the same changes as aerospike/aerospike-expression-lang-java#72