nfsantos commented on code in PR #2098:
URL: https://github.com/apache/jackrabbit-oak/pull/2098#discussion_r1961978908
##########
oak-search/src/main/java/org/apache/jackrabbit/oak/plugins/index/search/spi/editor/FulltextIndexEditor.java:
##########
@@ -41,361 +40,370 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import static org.apache.jackrabbit.oak.commons.PathUtils.concat;
-
/**
* Generic implementation of an {@link IndexEditor} which supports index time
aggregation.
*/
public class FulltextIndexEditor<D> implements IndexEditor,
Aggregate.AggregateRoot {
- private static final Logger log =
- LoggerFactory.getLogger(FulltextIndexEditor.class);
+ private static final Logger log =
LoggerFactory.getLogger(FulltextIndexEditor.class);
- public static final String TEXT_EXTRACTION_ERROR = "TextExtractionError";
+ public static final String TEXT_EXTRACTION_ERROR = "TextExtractionError";
- private final FulltextIndexEditorContext<D> context;
+ private static final List<Aggregate.Matcher> EMPTY_AGGREGATE_MATCHER_LIST
= List.of();
- /* Name of this node, or {@code null} for the root node. */
- private final String name;
+ private final FulltextIndexEditorContext<D> context;
- /* Parent editor or {@code null} if this is the root editor. */
- private final FulltextIndexEditor<D> parent;
+ /* Name of this node, or {@code null} for the root node. */
+ private final String name;
- /* Path of this editor, built lazily in {@link #getPath()}. */
- private String path;
+ /* Parent editor or {@code null} if this is the root editor. */
+ private final FulltextIndexEditor<D> parent;
- private boolean propertiesChanged = false;
+ /* Path of this editor, built lazily in {@link #getPath()}. */
+ private String path;
- private final List<PropertyState> propertiesModified = new ArrayList<>();
+ private boolean propertiesChanged = false;
- /*
- * Flag indicating if the current tree being traversed has a deleted parent.
- */
- private final boolean isDeleted;
+ private final List<PropertyState> propertiesModified = new ArrayList<>();
- private IndexDefinition.IndexingRule indexingRule;
+ /*
+ * Flag indicating if the current tree being traversed has a deleted
parent.
+ */
+ private final boolean isDeleted;
- private List<Aggregate.Matcher> currentMatchers = Collections.emptyList();
+ private IndexDefinition.IndexingRule indexingRule;
- private final MatcherState matcherState;
+ private List<Aggregate.Matcher> currentMatchers = List.of();
- private final PathFilter.Result pathFilterResult;
+ private final MatcherState matcherState;
- public FulltextIndexEditor(FulltextIndexEditorContext<D> context) {
- this.parent = null;
- this.name = null;
- this.path = "/";
- this.context = context;
- this.isDeleted = false;
- this.matcherState = MatcherState.NONE;
- this.pathFilterResult =
context.getDefinition().getPathFilter().filter(PathUtils.ROOT_PATH);
- }
+ private final PathFilter.Result pathFilterResult;
- public FulltextIndexEditor(FulltextIndexEditor<D> parent, String name,
- MatcherState matcherState,
- PathFilter.Result pathFilterResult,
- boolean isDeleted) {
- this.parent = parent;
- this.name = name;
- this.path = null;
- this.context = parent.context;
- this.isDeleted = isDeleted;
- this.matcherState = matcherState;
- this.pathFilterResult = pathFilterResult;
- }
+ public FulltextIndexEditor(FulltextIndexEditorContext<D> context) {
+ this.parent = null;
+ this.name = null;
+ this.path = "/";
+ this.context = context;
+ this.isDeleted = false;
+ this.matcherState = MatcherState.NONE;
+ this.pathFilterResult =
context.getDefinition().getPathFilter().filter(PathUtils.ROOT_PATH);
+ }
- public String getPath() {
- if (path == null) { // => parent != null
- path = concat(parent.getPath(), name);
+ public FulltextIndexEditor(FulltextIndexEditor<D> parent, String name,
+ MatcherState matcherState,
+ PathFilter.Result pathFilterResult,
+ boolean isDeleted) {
+ this.parent = parent;
+ this.name = name;
+ this.path = null;
+ this.context = parent.context;
+ this.isDeleted = isDeleted;
+ this.matcherState = matcherState;
+ this.pathFilterResult = pathFilterResult;
}
- return path;
- }
- @Override
- public void enter(NodeState before, NodeState after) {
- if (EmptyNodeState.MISSING_NODE == before && parent == null){
- context.enableReindexMode();
+ public String getPath() {
+ if (path == null) { // => parent != null
+ path = PathUtils.concat(parent.getPath(), name);
+ }
+ return path;
}
- //Only check for indexing if the result is include.
- //In case like TRAVERSE nothing needs to be indexed for those
- //path
- if (pathFilterResult == PathFilter.Result.INCLUDE) {
- //For traversal in deleted sub tree before state has to be used
- NodeState current = after.exists() ? after : before;
- indexingRule = getDefinition().getApplicableIndexingRule(current);
-
- if (indexingRule != null) {
- currentMatchers = indexingRule.getAggregate().createMatchers(this);
- }
+ @Override
+ public void enter(NodeState before, NodeState after) {
+ if (EmptyNodeState.MISSING_NODE == before && parent == null) {
+ context.enableReindexMode();
+ }
+
+ //Only check for indexing if the result is include.
+ //In case like TRAVERSE nothing needs to be indexed for those
+ //path
+ if (pathFilterResult == PathFilter.Result.INCLUDE) {
+ //For traversal in deleted sub tree before state has to be used
+ NodeState current = after.exists() ? after : before;
+ indexingRule = getDefinition().getApplicableIndexingRule(current);
+
+ if (indexingRule != null) {
+ currentMatchers =
indexingRule.getAggregate().createMatchers(this);
+ }
+ }
}
- }
-
- @Override
- public void leave(NodeState before, NodeState after)
- throws CommitFailedException {
- if (propertiesChanged || !before.exists()) {
- String path = getPath();
- if (addOrUpdate(path, after, before.exists())) {
- long indexed = context.incIndexedNodes();
- if (indexed % 1000 == 0) {
- log.debug("[{}] => Indexed {} nodes...", getIndexName(), indexed);
+
+ @Override
+ public void leave(NodeState before, NodeState after)
+ throws CommitFailedException {
+ if (propertiesChanged || !before.exists()) {
+ String path = getPath();
+ if (addOrUpdate(path, after, before.exists())) {
+ long indexed = context.incIndexedNodes();
+ if (indexed % 1000 == 0) {
+ log.debug("[{}] => Indexed {} nodes...", getIndexName(),
indexed);
+ }
+ }
+ }
+
+ if (!matcherState.affectedMatchers.isEmpty()) {
+ for (Aggregate.Matcher m : matcherState.affectedMatchers) {
+ m.markRootDirty();
+ }
+ }
+
+ if (parent == null) {
+ PropertyUpdateCallback callback =
context.getPropertyUpdateCallback();
+ if (callback != null) {
+ callback.done();
+ }
+
+ try {
+ context.closeWriter();
+ } catch (IOException e) {
+ CommitFailedException ce = new
CommitFailedException("Fulltext", 4,
+ "Failed to close the Fulltext index " +
context.getIndexingContext().getIndexPath(), e);
+ context.getIndexingContext().indexUpdateFailed(ce);
+ throw ce;
+ }
+ if (context.getIndexedNodes() > 0) {
+ log.debug("[{}] => Indexed {} nodes, done.", getIndexName(),
context.getIndexedNodes());
+ }
}
- }
}
- for (Aggregate.Matcher m : matcherState.affectedMatchers){
- m.markRootDirty();
+ @Override
+ public void propertyAdded(PropertyState after) {
+ markPropertyChanged(after.getName());
+ checkAggregates(after.getName());
+ propertyUpdated(null, after);
}
- if (parent == null) {
- PropertyUpdateCallback callback = context.getPropertyUpdateCallback();
- if (callback != null) {
- callback.done();
- }
-
- try {
- context.closeWriter();
- } catch (IOException e) {
- CommitFailedException ce = new CommitFailedException("Fulltext", 4,
- "Failed to close the Fulltext index " +
context.getIndexingContext().getIndexPath(), e);
- context.getIndexingContext().indexUpdateFailed(ce);
- throw ce;
- }
- if (context.getIndexedNodes() > 0) {
- log.debug("[{}] => Indexed {} nodes, done.", getIndexName(),
context.getIndexedNodes());
- }
+ @Override
+ public void propertyChanged(PropertyState before, PropertyState after) {
+ markPropertyChanged(before.getName());
+ propertiesModified.add(before);
+ checkAggregates(before.getName());
+ propertyUpdated(before, after);
}
- }
-
- @Override
- public void propertyAdded(PropertyState after) {
- markPropertyChanged(after.getName());
- checkAggregates(after.getName());
- propertyUpdated(null, after);
- }
-
- @Override
- public void propertyChanged(PropertyState before, PropertyState after) {
- markPropertyChanged(before.getName());
- propertiesModified.add(before);
- checkAggregates(before.getName());
- propertyUpdated(before, after);
- }
-
- @Override
- public void propertyDeleted(PropertyState before) {
- markPropertyChanged(before.getName());
- propertiesModified.add(before);
- checkAggregates(before.getName());
- propertyUpdated(before, null);
- }
-
- @Override
- public Editor childNodeAdded(String name, NodeState after) {
- PathFilter.Result filterResult = getPathFilterResult(name);
- if (filterResult != PathFilter.Result.EXCLUDE) {
- return new FulltextIndexEditor<>(this, name, getMatcherState(name,
after), filterResult, false);
+
+ @Override
+ public void propertyDeleted(PropertyState before) {
+ markPropertyChanged(before.getName());
+ propertiesModified.add(before);
+ checkAggregates(before.getName());
+ propertyUpdated(before, null);
}
- return null;
- }
-
- @Override
- public Editor childNodeChanged(
- String name, NodeState before, NodeState after) {
- PathFilter.Result filterResult = getPathFilterResult(name);
- if (filterResult != PathFilter.Result.EXCLUDE) {
- return new FulltextIndexEditor<>(this, name, getMatcherState(name,
after), filterResult, false);
+
+ @Override
+ public Editor childNodeAdded(String name, NodeState after) {
+ PathFilter.Result filterResult = getPathFilterResult(name);
+ if (filterResult != PathFilter.Result.EXCLUDE) {
+ return new FulltextIndexEditor<>(this, name, getMatcherState(name,
after), filterResult, false);
+ }
+ return null;
}
- return null;
- }
-
- @Override
- public Editor childNodeDeleted(String name, NodeState before)
- throws CommitFailedException {
- PathFilter.Result filterResult = getPathFilterResult(name);
- if (filterResult == PathFilter.Result.EXCLUDE) {
- return null;
+
+ @Override
+ public Editor childNodeChanged(String name, NodeState before, NodeState
after) {
+ PathFilter.Result filterResult = getPathFilterResult(name);
+ if (filterResult != PathFilter.Result.EXCLUDE) {
+ return new FulltextIndexEditor<>(this, name, getMatcherState(name,
after), filterResult, false);
+ }
+ return null;
}
- if (!isDeleted) {
- // tree deletion is handled on the parent node
- String path = concat(getPath(), name);
- try {
- FulltextIndexWriter<D> writer = context.getWriter();
- // Remove all index entries in the removed subtree
- writer.deleteDocuments(path);
- this.context.indexUpdate();
- } catch (IOException e) {
- CommitFailedException ce = new CommitFailedException("Fulltext", 5,
"Failed to remove the index entries of"
- + " the removed subtree " + path + "for index " +
context.getIndexingContext().getIndexPath(), e);
- context.getIndexingContext().indexUpdateFailed(ce);
- throw ce;
- }
+ @Override
+ public Editor childNodeDeleted(String name, NodeState before)
+ throws CommitFailedException {
+ PathFilter.Result filterResult = getPathFilterResult(name);
+ if (filterResult == PathFilter.Result.EXCLUDE) {
+ return null;
+ }
+
+ if (!isDeleted) {
+ // tree deletion is handled on the parent node
+ String path = PathUtils.concat(getPath(), name);
+ try {
+ FulltextIndexWriter<D> writer = context.getWriter();
+ // Remove all index entries in the removed subtree
+ writer.deleteDocuments(path);
+ this.context.indexUpdate();
+ } catch (IOException e) {
+ CommitFailedException ce = new
CommitFailedException("Fulltext", 5, "Failed to remove the index entries of"
+ + " the removed subtree " + path + "for index " +
context.getIndexingContext().getIndexPath(), e);
+ context.getIndexingContext().indexUpdateFailed(ce);
+ throw ce;
+ }
+ }
+
+ MatcherState ms = getMatcherState(name, before);
+ if (!ms.isEmpty()) {
+ return new FulltextIndexEditor<>(this, name, ms, filterResult,
true);
+ }
+ return null; // no need to recurse down the removed subtree
}
- MatcherState ms = getMatcherState(name, before);
- if (!ms.isEmpty()){
- return new FulltextIndexEditor<>(this, name, ms, filterResult, true);
+ public FulltextIndexEditorContext<D> getContext() {
+ return context;
}
- return null; // no need to recurse down the removed subtree
- }
-
- public FulltextIndexEditorContext<D> getContext() {
- return context;
- }
-
- private boolean addOrUpdate(String path, NodeState state, boolean isUpdate)
- throws CommitFailedException {
- try {
- D d = makeDocument(path, state, isUpdate);
- if (d != null) {
- if (log.isTraceEnabled()) {
- log.trace("[{}] Indexed document for {} is {}", getIndexName(),
path, d);
+
+ private boolean addOrUpdate(String path, NodeState state, boolean isUpdate)
+ throws CommitFailedException {
+ try {
+ D d = makeDocument(path, state, isUpdate);
+ if (d != null) {
+ if (log.isTraceEnabled()) {
+ log.trace("[{}] Indexed document for {} is {}",
getIndexName(), path, d);
+ }
+ context.indexUpdate();
+ context.getWriter().updateDocument(path, d);
+ return true;
+ }
+ } catch (IOException e) {
+ log.warn("Failed to index the node [{}] due to {}", path,
e.getMessage());
+ CommitFailedException ce = new CommitFailedException("Fulltext", 3,
+ "Failed to index the node " + path, e);
+ context.getIndexingContext().indexUpdateFailed(ce);
+ throw ce;
+ } catch (IllegalArgumentException ie) {
+ log.warn("Failed to index the node [{}]", path, ie);
}
- context.indexUpdate();
- context.getWriter().updateDocument(path, d);
- return true;
- }
- } catch (IOException e) {
- log.warn("Failed to index the node [{}] due to {}", path,
e.getMessage());
- CommitFailedException ce = new CommitFailedException("Fulltext", 3,
- "Failed to index the node " + path, e);
- context.getIndexingContext().indexUpdateFailed(ce);
- throw ce;
- } catch (IllegalArgumentException ie) {
- log.warn("Failed to index the node [{}]", path, ie);
+ return false;
}
- return false;
- }
- private D makeDocument(String path, NodeState state, boolean isUpdate)
throws IOException {
- if (!isIndexable()) {
- return null;
+ private D makeDocument(String path, NodeState state, boolean isUpdate)
throws IOException {
+ if (!isIndexable()) {
+ return null;
+ }
+ return context.newDocumentMaker(indexingRule,
path).makeDocument(state, isUpdate, propertiesModified);
}
- return context.newDocumentMaker(indexingRule, path).makeDocument(state,
isUpdate, propertiesModified);
- }
- //~-------------------------------------------------------< Aggregate >
+ //~-------------------------------------------------------< Aggregate >
- @Override
- public void markDirty() {
- propertiesChanged = true;
- }
+ @Override
+ public void markDirty() {
+ propertiesChanged = true;
+ }
- private MatcherState getMatcherState(String name, NodeState after) {
- List<Aggregate.Matcher> matched = new ArrayList<>();
- List<Aggregate.Matcher> inherited = new ArrayList<>();
- for (Aggregate.Matcher m : Iterables.concat(matcherState.inherited,
currentMatchers)) {
- Aggregate.Matcher result = m.match(name, after);
- if (result.getStatus() == Aggregate.Matcher.Status.MATCH_FOUND){
- matched.add(result);
- }
+ private MatcherState getMatcherState(String name, NodeState after) {
+ List<Aggregate.Matcher> matched = EMPTY_AGGREGATE_MATCHER_LIST;
+ List<Aggregate.Matcher> inherited = EMPTY_AGGREGATE_MATCHER_LIST;
+ for (Aggregate.Matcher m : Iterables.concat(matcherState.inherited,
currentMatchers)) {
+ Aggregate.Matcher result = m.match(name, after);
+ if (result.getStatus() == Aggregate.Matcher.Status.MATCH_FOUND) {
+ if (matched == EMPTY_AGGREGATE_MATCHER_LIST) {
+ matched = new ArrayList<>(2);
+ }
+ matched.add(result);
+ }
+
+ if (result.getStatus() != Aggregate.Matcher.Status.FAIL) {
+ if (inherited == EMPTY_AGGREGATE_MATCHER_LIST) {
+ inherited = new ArrayList<>(2);
+ }
+ inherited.addAll(result.nextSet());
+ }
+ }
- if (result.getStatus() != Aggregate.Matcher.Status.FAIL){
- inherited.addAll(result.nextSet());
- }
+ if (!matched.isEmpty() || !inherited.isEmpty()) {
+ return new MatcherState(matched, inherited);
+ }
+ return MatcherState.NONE;
Review Comment:
Yes, cleaner to read. I don't think it will improve performance, it seems to
be equivalent, but I will change it because it is nicer.
--
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: oak-dev-unsubscr...@jackrabbit.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
