fabriziofortino commented on code in PR #679:
URL: https://github.com/apache/jackrabbit-oak/pull/679#discussion_r957131789
##########
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 agree. In this case, the ignore test was removed because it no longer
fails thanks to the change in `FacetCommonTest`.
##########
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:
done
##########
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:
The comment was a leftover.
I personally like concise code but I see it could be less readable. I have
made it a little more verbose. Hope is a good compromise.
##########
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:
In Elastic, since the beginning, we have decided to always store the
ancestors since the savings in size are minimal.
I have decided to change the query time behavior in this patch to reduce the
differences with Lucene. Otherwise, all the common tests we have would fail.
I find this flag quite confusing: if `evaluatePathRestrictions` a query
using `isDescendantNode` would not fail but it will return more results
_silently_. I would be in favor of deprecating the use of the flag everywhere
to simplify the configuration since the gain is not noticeable.
##########
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:
This is a fixed value as reported in the documentation:
```
Note, the subset is done by filtering top 10 spellchecks. So, it's possible
to get no results for a subtree query, if top 10 spellchecks are not part of
that subtree.
```
https://jackrabbit.apache.org/oak/docs/query/lucene.html#spellchecking
--
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]
