nfsantos commented on code in PR #2346:
URL: https://github.com/apache/jackrabbit-oak/pull/2346#discussion_r2161511697
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/ElasticRequestHandler.java:
##########
@@ -1123,7 +1127,20 @@ private Query createQuery(String propertyName,
Filter.PropertyRestriction pr, Pr
final String field =
elasticIndexDefinition.getElasticKeyword(propertyName);
if (pr.isNullRestriction()) {
- return Query.of(q -> q.bool(b -> b.mustNot(m -> m.exists(e ->
e.field(field)))));
+ // nullProps check has been added since 1.4.0. Use the old
strategy when version is lower
+ if
(elasticIndexDefinition.getMappingVersion().compareTo(MINIMUM_NULL_CHECK_VERSION)
< 0) {
+ // check if the default mapping is >= 1.5.0
+ if (ElasticIndexDefinition.MAPPING_VERSION != null &&
+ ElasticIndexDefinition.MAPPING_VERSION.compareTo(new
ElasticSemVer(1, 5, 0)) >= 0) {
Review Comment:
Suggestion: create a constant with `new ElasticSemVer(1, 5, 0)`: avoids
creating a new instance every time this logic is executed and improves
readability because the name of the constant can document the meaning of this
particular version (maybe `MAXIMUM_NULL_CHECK_VERSION`?)
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocument.java:
##########
@@ -79,6 +81,7 @@ public class ElasticDocument {
this.fulltext = new LinkedHashSet<>();
this.suggest = new LinkedHashSet<>();
this.spellcheck = new LinkedHashSet<>();
+ this.nullProperties = new LinkedHashSet<>();
this.properties = new HashMap<>();
Review Comment:
Just a side comment, not related to this PR. All the other datastructures
used for the fields in this class have a well-defined iteration order, except
this one. Could this be an issue? If so, for consistency with the other fields,
we could use a `LinkedHashMap` here. Or if the order is not important, then we
can use `HashSet` for the others instead of `LinkedHashSet`.
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/ElasticRequestHandler.java:
##########
@@ -1123,7 +1127,20 @@ private Query createQuery(String propertyName,
Filter.PropertyRestriction pr, Pr
final String field =
elasticIndexDefinition.getElasticKeyword(propertyName);
if (pr.isNullRestriction()) {
- return Query.of(q -> q.bool(b -> b.mustNot(m -> m.exists(e ->
e.field(field)))));
+ // nullProps check has been added since 1.4.0. Use the old
strategy when version is lower
Review Comment:
```suggestion
// nullProps check has been added in 1.4.0. Use the old strategy
when version is lower
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
