thomasmueller commented on code in PR #679:
URL: https://github.com/apache/jackrabbit-oak/pull/679#discussion_r957081959
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentMaker.java:
##########
@@ -53,7 +53,8 @@ protected ElasticDocument initDoc() {
@Override
protected ElasticDocument finalizeDoc(ElasticDocument doc, boolean dirty,
boolean facet) {
- // evaluate path restrictions is enabled by default in elastic. Always
index ancestors
+ // Evaluate path restrictions is enabled by default in elastic. Always
index ancestors.
+ // When specifically disabled, we will keep indexing it, but the field
won't be used at query time
Review Comment:
It's very good to have such documentation!
.... I wonder if we could also document the reason for this behavior... E.g.
we believe that at some point, we will deprecate the ability to _disable_
"evaluate path restrictions", or something like that.
And, why do we change the behavior in this patch? It doesn't sound related
to the issue (but maybe I'm wrong, in which case we should document this in the
issue).
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/ElasticRequestHandler.java:
##########
@@ -443,7 +445,7 @@ public PhraseSuggester suggestQuery() {
return PhraseSuggester.of(ps -> ps
.field(FieldNames.SPELLCHECK)
.size(10)
- .directGenerator(d ->
d.field(FieldNames.SPELLCHECK).suggestMode(SuggestMode.Missing))
+ .directGenerator(d ->
d.field(FieldNames.SPELLCHECK).suggestMode(SuggestMode.Missing).size(10))
Review Comment:
Could we document the "size(10)" part? I'm not sure if I understand... does
it mean that we return a maximum of 10 results instead of 5? If yes why is 10
good and 5 bad, and would it make sense to document this or is it already
documented (e.g. in the Lucene documentation)? Then we could point to that
documentation.
But maybe I just completly misunderstand the "10" here.
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/ElasticRequestHandler.java:
##########
@@ -719,24 +720,24 @@ public Highlight highlight() {
return ":fulltext";
}
- private static Query nodeTypeConstraints(IndexDefinition.IndexingRule
defn, Filter filter) {
- final BoolQuery.Builder bqBuilder = new BoolQuery.Builder();
+ private static Optional<Query>
nodeTypeConstraints(IndexDefinition.IndexingRule defn, Filter filter) {
+ List<Query> queries = new ArrayList<>();
PropertyDefinition primaryType = defn.getConfig(JCR_PRIMARYTYPE);
// TODO OAK-2198 Add proper nodeType query support
if (primaryType != null && primaryType.propertyIndex) {
for (String type : filter.getPrimaryTypes()) {
- bqBuilder.should(q -> q.term(t ->
t.field(JCR_PRIMARYTYPE).value(FieldValue.of(type))));
+ queries.add(TermQuery.of(t ->
t.field(JCR_PRIMARYTYPE).value(FieldValue.of(type)))._toQuery());
}
}
PropertyDefinition mixinType = defn.getConfig(JCR_MIXINTYPES);
if (mixinType != null && mixinType.propertyIndex) {
for (String type : filter.getMixinTypes()) {
- bqBuilder.should(q -> q.term(t ->
t.field(JCR_MIXINTYPES).value(FieldValue.of(type))));
+ queries.add(TermQuery.of(t ->
t.field(JCR_MIXINTYPES).value(FieldValue.of(type)))._toQuery());
}
}
- return Query.of(q -> q.bool(bqBuilder.build()));
+ return Optional.ofNullable(queries.isEmpty() ? null : Query.of(qb ->
qb.bool(b -> b.should(queries))));
Review Comment:
What about the alternative:
```
// your code
return Optional.ofNullable(queries.isEmpty() ? null : Query.of(qb ->
qb.bool(b -> b.should(queries))));
// alternative
return queries.isEmpty() ? Optional.empty() : Query.of(qb -> qb.bool(b ->
b.should(queries)));
```
It would be a bit shorter, and in my view easier to understand.
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/ElasticRequestHandler.java:
##########
@@ -573,54 +575,53 @@ private List<Query> nonFullTextConstraints(IndexPlan
plan, PlanResult planResult
Filter filter = plan.getFilter();
if (!filter.matchesAllTypes()) {
- queries.add(nodeTypeConstraints(planResult.indexingRule, filter));
+ nodeTypeConstraints(planResult.indexingRule,
filter).ifPresent(queries::add);
+ //queries.add(nodeTypeConstraints(planResult.indexingRule,
filter));
Review Comment:
Two remarks:
* Why do we comment the source code here?
* I personally find it easier to read to use
```
x = nodeTypeConstraints(planResult.indexingRule, filter);
if (x.isPresent()) {
queries.add(x);
}
```
But it's a matter of taste.
##########
oak-lucene/src/test/java/org/apache/jackrabbit/oak/plugins/index/lucene/LuceneFacetCommonTest.java:
##########
@@ -46,13 +44,6 @@ protected Repository createJcrRepository() {
return jcr.createRepository();
}
- @Override
- @Test
- @Ignore("failing in lucene only, needs more investigation")
Review Comment:
I think best practise is to use the oak issue in the Ignore annotation
("OAK-9912"?)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
