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: dev-unsubscr...@jackrabbit.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org