Skip to content

Commit

Permalink
OAK-9572: index aggregations should ignore hidden properties (apache#368
Browse files Browse the repository at this point in the history 
)

* OAK-9572: skip hidden properties while collecting aggregations

* OAK-9572: (improvements) reduced use of guava

* OAK-9572: add unit test

* OAK-9572: (improvements) cleanup AggregateTest

* OAK-9572: (minor) fixed modifier

* OAK-9572: (test) include also hidden nodes
  • Loading branch information
@fabriziofortino
fabriziofortino authored Sep 14, 2021
1 parent c37f1d4 commit 52bb236
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 105 deletions.
61 changes: 25 additions & 36 deletions ...st/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticDynamicBoostQueryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,14 @@
import org.junit.Test;

import java.io.ByteArrayInputStream;
import java.io.InputStream;
import java.util.Arrays;
import java.util.UUID;

import static org.apache.jackrabbit.JcrConstants.NT_UNSTRUCTURED;

public class ElasticDynamicBoostQueryTest extends ElasticAbstractQueryTest {

public static final String ASSET_NODE_TYPE =
private static final String ASSET_NODE_TYPE =
"[dam:Asset]\n" +
" - * (UNDEFINED) multiple\n" +
" - * (UNDEFINED)\n" +
Expand All @@ -47,20 +46,16 @@ public void dynamicBoost() throws CommitFailedException {

Tree test = createNodeWithType(root.getTree("/"), "test", NT_UNSTRUCTURED);
Tree item1Metadata = createNodeWithMetadata(test, "item1", "flower with a lot of red and a bit of blue");
Tree item1Color1 = createNodeWithType(item1Metadata,"color1", NT_UNSTRUCTURED);
item1Color1.setProperty("name", "red");
item1Color1.setProperty("confidence", 9.0);
Tree item1Color2 = createNodeWithType(item1Metadata,"color2", NT_UNSTRUCTURED);
item1Color2.setProperty("name", "blue");
item1Color2.setProperty("confidence", 1.0);
Tree item1Color1 = createNodeWithType(item1Metadata, "color1", NT_UNSTRUCTURED);
configureBoostedField(item1Color1, "red", 9.0);
Tree item1Color2 = createNodeWithType(item1Metadata, "color2", NT_UNSTRUCTURED);
configureBoostedField(item1Color2, "blue", 1.0);

Tree item2Metadata = createNodeWithMetadata(test, "item2", "flower with a lot of blue and a bit of red");
Tree item2Color1 = createNodeWithType(item2Metadata,"color1", NT_UNSTRUCTURED);
item2Color1.setProperty("name", "blue");
item2Color1.setProperty("confidence", 9.0);
Tree item2Color2 = createNodeWithType(item2Metadata,"color2", NT_UNSTRUCTURED);
item2Color2.setProperty("name", "red");
item2Color2.setProperty("confidence", 1.0);
Tree item2Color1 = createNodeWithType(item2Metadata, "color1", NT_UNSTRUCTURED);
configureBoostedField(item2Color1, "blue", 9.0);
Tree item2Color2 = createNodeWithType(item2Metadata, "color2", NT_UNSTRUCTURED);
configureBoostedField(item2Color2, "red", 1.0);
root.commit();

assertEventually(() -> {
Expand All @@ -81,19 +76,15 @@ public void dynamicBoostAnalyzed() throws CommitFailedException {
Tree item1Metadata = createNodeWithMetadata(test, "item1", "flower with a lot of red and a bit of blue");
item1Metadata.setProperty("foo", "bar");
Tree item1Color1 = createNodeWithType(item1Metadata,"color1", NT_UNSTRUCTURED);
item1Color1.setProperty("name", "red");
item1Color1.setProperty("confidence", 9.0);
configureBoostedField(item1Color1, "red", 9.0);
Tree item1Color2 = createNodeWithType(item1Metadata,"color2", NT_UNSTRUCTURED);
item1Color2.setProperty("name", "blue");
item1Color2.setProperty("confidence", 1.0);
configureBoostedField(item1Color2, "blue", 1.0);

Tree item2Metadata = createNodeWithMetadata(test, "item2", "flower with a lot of blue and a bit of red");
Tree item2Color1 = createNodeWithType(item2Metadata,"color1", NT_UNSTRUCTURED);
item2Color1.setProperty("name", "blue");
item2Color1.setProperty("confidence", 9.0);
configureBoostedField(item2Color1, "blue", 9.0);
Tree item2Color2 = createNodeWithType(item2Metadata,"color2", NT_UNSTRUCTURED);
item2Color2.setProperty("name", "red");
item2Color2.setProperty("confidence", 1.0);
configureBoostedField(item2Color2, "red", 1.0);
root.commit();

assertEventually(() -> {
Expand All @@ -113,19 +104,15 @@ public void dynamicBoostWithAdditionalTags() throws CommitFailedException {
Tree test = createNodeWithType(root.getTree("/"), "test", NT_UNSTRUCTURED);
Tree item1Metadata = createNodeWithMetadata(test, "item1", "flower with a lot of colors");
Tree item1Color1 = createNodeWithType(item1Metadata,"color1", NT_UNSTRUCTURED);
item1Color1.setProperty("name", "red");
item1Color1.setProperty("confidence", 9.0);
configureBoostedField(item1Color1, "red", 9.0);
Tree item1Color2 = createNodeWithType(item1Metadata,"color2", NT_UNSTRUCTURED);
item1Color2.setProperty("name", "blue");
item1Color2.setProperty("confidence", 1.0);
configureBoostedField(item1Color2, "blue", 1.0);

Tree item2Metadata = createNodeWithMetadata(test, "item2", "flower with a lot of colors");
Tree item2Color1 = createNodeWithType(item2Metadata,"color1", NT_UNSTRUCTURED);
item2Color1.setProperty("name", "blue");
item2Color1.setProperty("confidence", 9.0);
configureBoostedField(item2Color1, "blue", 9.0);
Tree item2Color2 = createNodeWithType(item2Metadata,"color2", NT_UNSTRUCTURED);
item2Color2.setProperty("name", "red");
item2Color2.setProperty("confidence", 1.0);
configureBoostedField(item2Color2, "red", 1.0);
root.commit();

assertEventually(() -> {
Expand All @@ -138,8 +125,14 @@ public void dynamicBoostWithAdditionalTags() throws CommitFailedException {
});
}

private void configureBoostedField(Tree node, String name, double confidence) {
node.setProperty("name", name);
node.orderBefore(null);
node.setProperty("confidence", confidence);
}

private void configureIndex() throws CommitFailedException {
NodeTypeRegistry.register(root, toInputStream(ASSET_NODE_TYPE), "test nodeType");
NodeTypeRegistry.register(root, new ByteArrayInputStream(ASSET_NODE_TYPE.getBytes()), "test nodeType");
IndexDefinitionBuilder builder = createIndex(true, "dam:Asset", "title", "dynamicBoost");
IndexDefinitionBuilder.PropertyRule title = builder.indexRule("dam:Asset")
.property("title")
Expand All @@ -164,13 +157,9 @@ private Tree createNodeWithMetadata(Tree parent, String nodeName, String title)
"metadata", NT_UNSTRUCTURED);
}

private static Tree createNodeWithType(Tree t, String nodeName, String typeName){
private Tree createNodeWithType(Tree t, String nodeName, String typeName){
t = t.addChild(nodeName);
t.setProperty(JcrConstants.JCR_PRIMARYTYPE, typeName, Type.NAME);
return t;
}

private static InputStream toInputStream(String x) {
return new ByteArrayInputStream(x.getBytes());
}
}
53 changes: 22 additions & 31 deletions oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/Aggregate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,30 +19,27 @@

package org.apache.jackrabbit.oak.plugins.index.search;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.regex.Pattern;

import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import org.apache.jackrabbit.oak.api.PropertyState;
import org.apache.jackrabbit.oak.commons.PathUtils;
import org.apache.jackrabbit.oak.plugins.index.search.util.ConfigUtil;
import org.apache.jackrabbit.oak.plugins.memory.MemoryChildNodeEntry;
import org.apache.jackrabbit.oak.spi.state.ChildNodeEntry;
import org.apache.jackrabbit.oak.spi.state.NodeState;
import org.apache.jackrabbit.oak.spi.state.NodeStateUtils;
import org.jetbrains.annotations.Nullable;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.collect.Iterables.toArray;
import static com.google.common.collect.Lists.newArrayList;
import static com.google.common.collect.Lists.newArrayListWithCapacity;
import static org.apache.jackrabbit.oak.commons.PathUtils.elements;
import static org.apache.jackrabbit.oak.commons.PathUtils.getParentPath;

Expand All @@ -53,7 +50,7 @@
* stored in child nodes; say /x/section1 contains "Hello" and /x/section2
* contains "World". If index aggregation is configured correctly, it will
* combine all the text of the child nodes, and index that as /x. When doing a
* fulltext search for for "Hello World", the index will then return /x.
* fulltext search for "Hello World", the index will then return /x.
*/
public class Aggregate {

Expand All @@ -80,10 +77,10 @@ public Aggregate(String nodeTypeName, List<? extends Include> includes) {
Aggregate(String nodeTypeName, List<? extends Include> includes,
int recursionLimit) {
this.nodeTypeName = nodeTypeName;
this.includes = ImmutableList.copyOf(includes);
this.includes = Collections.unmodifiableList(includes);
this.reAggregationLimit = recursionLimit;
this.relativeNodeIncludes = findRelativeNodeIncludes(includes);
this.nodeAggregates = hasNodeIncludes(includes);
this.nodeAggregates = includes.stream().anyMatch(input -> input instanceof NodeInclude);
}

public List<? extends Include> getIncludes() {
Expand All @@ -98,7 +95,7 @@ public void collectAggregates(NodeState root, ResultCollector collector) {
}

public List<Matcher> createMatchers(AggregateRoot root){
List<Matcher> matchers = newArrayListWithCapacity(includes.size());
List<Matcher> matchers = new ArrayList<>(includes.size());
for (Include include : includes) {
matchers.add(new Matcher(this, include, root));
}
Expand Down Expand Up @@ -147,7 +144,7 @@ private static void collectAggregates(NodeState nodeState, List<Matcher> matcher

private static void collectAggregatesForDirectMatchers(NodeState nodeState, List<Matcher> matchers,
ResultCollector collector) {
Map<String, ChildNodeEntry> children = Maps.newHashMap();
Map<String, ChildNodeEntry> children = new HashMap<>();
//Collect potentially matching child nodestates based on matcher name
for (Matcher m : matchers){
String nodeName = m.getNodeName();
Expand All @@ -167,7 +164,7 @@ private static void collectAggregatesForPatternMatchers(NodeState nodeState, Lis
private static void matchChildren(List<Matcher> matchers, ResultCollector collector,
Iterable<? extends ChildNodeEntry> children) {
for (ChildNodeEntry cne : children) {
List<Matcher> nextSet = newArrayListWithCapacity(matchers.size());
List<Matcher> nextSet = new ArrayList<>(matchers.size());
for (Matcher m : matchers) {
Matcher result = m.match(cne.getName(), cne.getNodeState());
if (result.getStatus() == Matcher.Status.MATCH_FOUND){
Expand All @@ -194,15 +191,15 @@ private static boolean hasPatternMatcher(List<Matcher> matchers){
}

private List<Matcher> createMatchers() {
List<Matcher> matchers = newArrayListWithCapacity(includes.size());
List<Matcher> matchers = new ArrayList<>(includes.size());
for (Include include : includes) {
matchers.add(new Matcher(this, include));
}
return matchers;
}

private static List<NodeInclude> findRelativeNodeIncludes(List<? extends Include> includes) {
List<NodeInclude> result = newArrayList();
List<NodeInclude> result = new ArrayList<>();
for (Include i : includes){
if (i instanceof NodeInclude){
NodeInclude ni = (NodeInclude) i;
Expand All @@ -211,16 +208,7 @@ private static List<NodeInclude> findRelativeNodeIncludes(List<? extends Include
}
}
}
return ImmutableList.copyOf(result);
}

private static boolean hasNodeIncludes(List<? extends Include> includes) {
return Iterables.any(includes, new Predicate<Include>() {
@Override
public boolean apply(Include input) {
return input instanceof NodeInclude;
}
});
return Collections.unmodifiableList(result);
}

public interface AggregateMapper {
Expand Down Expand Up @@ -393,17 +381,20 @@ public PropertyInclude(PropertyDefinition pd) {
}
}

/**
* Collect the aggregated results ignoring hidden properties
*/
@Override
public void collectResults(String nodePath, NodeState nodeState, ResultCollector results) {
if (pattern != null) {
for (PropertyState ps : nodeState.getProperties()) {
if (pattern.matcher(ps.getName()).matches()) {
if (!NodeStateUtils.isHidden(ps.getName()) && pattern.matcher(ps.getName()).matches()) {
results.onResult(new PropertyIncludeResult(ps, propertyDefinition, parentPath));
}
}
} else {
PropertyState ps = nodeState.getProperty(propertyName);
if (ps != null) {
if (ps != null && !NodeStateUtils.isHidden(ps.getName())) {
results.onResult(new PropertyIncludeResult(ps, propertyDefinition, parentPath));
}
}
Expand Down Expand Up @@ -568,9 +559,9 @@ private Matcher(Matcher m, Include i, String currentPath) {
this.matchedNodeState = null;
this.currentPath = currentPath;

List<String> paths = newArrayList(m.aggregateStack);
List<String> paths = new ArrayList<>(m.aggregateStack);
paths.add(currentPath);
this.aggregateStack = ImmutableList.copyOf(paths);
this.aggregateStack = Collections.unmodifiableList(paths);
}

public boolean isPatternBased() {
Expand Down Expand Up @@ -610,7 +601,7 @@ public Collection<Matcher> nextSet() {
return Collections.emptyList();
}

List<Matcher> result = Lists.newArrayListWithCapacity(nextAgg.includes.size());
List<Matcher> result = new ArrayList<>(nextAgg.includes.size());
for (Include i : nextAgg.includes){
result.add(new Matcher(this, i, currentPath));
}
Expand All @@ -626,8 +617,8 @@ public Collection<Matcher> nextSet() {
public void collectResults(ResultCollector results) {
checkArgument(status == Status.MATCH_FOUND);

//If result being collected as part of reaggregation then take path
//from the stack otherwise its the current path
//If result being collected as part of re-aggregation then take path
//from the stack otherwise it's the current path
String rootIncludePath = aggregateStack.isEmpty() ? currentPath : aggregateStack.get(0);
currentInclude.collectResults(rootState.rootInclude, rootIncludePath,
currentPath, matchedNodeState, results);
Expand Down
Loading

0 comments on commit 52bb236

Please sign in to comment.