thomasmueller commented on code in PR #2113:
URL: https://github.com/apache/jackrabbit-oak/pull/2113#discussion_r1975033753
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/Aggregate.java:
##########
@@ -132,82 +126,90 @@ private static boolean matchingType(String nodeTypeName,
NodeState nodeState) {
return false;
}
- private static void collectAggregates(NodeState nodeState, List<Matcher>
matchers,
- ResultCollector collector) {
- if (hasPatternMatcher(matchers)){
+ private static void collectAggregates(NodeState nodeState, Matcher[]
matchers, ResultCollector collector) {
+ if (hasPatternMatcher(matchers)) {
collectAggregatesForPatternMatchers(nodeState, matchers,
collector);
} else {
collectAggregatesForDirectMatchers(nodeState, matchers, collector);
}
}
- private static void collectAggregatesForDirectMatchers(NodeState
nodeState, List<Matcher> matchers,
+ private static void collectAggregatesForDirectMatchers(NodeState
nodeState, Matcher[] matchers,
ResultCollector
collector) {
- Map<String, ChildNodeEntry> children = new HashMap<>();
+ Map<String, ChildNodeEntry> children = null;
//Collect potentially matching child nodestates based on matcher name
- for (Matcher m : matchers){
+ for (Matcher m : matchers) {
String nodeName = m.getNodeName();
NodeState child = nodeState.getChildNode(nodeName);
- if (child.exists()){
+ if (child.exists()) {
+ if (children == null) {
+ children = new HashMap<>();
+ }
children.put(nodeName, new MemoryChildNodeEntry(nodeName,
child));
}
}
- matchChildren(matchers, collector, children.values());
+ if (children != null) {
+ matchChildren(matchers, collector, children.values());
+ }
}
- private static void collectAggregatesForPatternMatchers(NodeState
nodeState, List<Matcher> matchers,
+ private static void collectAggregatesForPatternMatchers(NodeState
nodeState, Matcher[] matchers,
ResultCollector
collector) {
matchChildren(matchers, collector, nodeState.getChildNodeEntries());
}
- private static void matchChildren(List<Matcher> matchers, ResultCollector
collector,
+ private static void matchChildren(Matcher[] matchers, ResultCollector
collector,
Iterable<? extends ChildNodeEntry>
children) {
+ List<Matcher> nextSet = null;
for (ChildNodeEntry cne : children) {
- 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){
+ if (result.getStatus() == Matcher.Status.MATCH_FOUND) {
result.collectResults(collector);
}
- if (result.getStatus() != Matcher.Status.FAIL){
- nextSet.addAll(result.nextSet());
+ if (result.getStatus() != Matcher.Status.FAIL) {
+ if (nextSet == null) {
+ nextSet = new ArrayList<>();
+ }
+ result.nextSet(nextSet);
}
}
- if (!nextSet.isEmpty()) {
- collectAggregates(cne.getNodeState(), nextSet, collector);
+ if (nextSet !=null && !nextSet.isEmpty()) {
+ collectAggregates(cne.getNodeState(), nextSet.toArray(new
Matcher[0]), collector);
+ nextSet.clear();
Review Comment:
I had some trouble understanding why nextSet.clear() is needed... Yes, it is
correct and matches the previous logic: before, a new ArrayList was created
inside the the children loop. Now, this is outside the loop.
Yes, it is probably faster that way, but I'm worried that the logic is
harder to understand, and in the future might cause some trouble... Not now...
now the code is fine! But after some more changes.
--
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]
