Copilot commented on code in PR #18759:
URL: https://github.com/apache/pinot/pull/18759#discussion_r3411739545
##########
pinot-core/src/main/java/org/apache/pinot/core/operator/filter/predicate/PredicateEvaluatorProvider.java:
##########
@@ -27,14 +27,17 @@
import org.apache.pinot.common.request.context.predicate.Predicate;
import org.apache.pinot.common.request.context.predicate.RangePredicate;
import org.apache.pinot.common.request.context.predicate.RegexpLikePredicate;
+import org.apache.pinot.common.utils.config.QueryOptionsUtils;
import org.apache.pinot.core.query.request.context.QueryContext;
+import
org.apache.pinot.segment.local.utils.fst.FSTTraversalLimitExceededException;
Review Comment:
`pinot-core` now directly depends on an exception type in
`pinot-segment-local` (`org.apache.pinot.segment.local...`). Since the
traversal limit is part of the `TextIndexReader`/predicate-evaluation contract
and is caught at the core layer, it would be cleaner to move
`FSTTraversalLimitExceededException` to a shared API module (e.g.,
`pinot-segment-spi` or `pinot-spi`) to avoid cross-module layering leaks and
make it usable by any `TextIndexReader` implementation.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/fst/RegexpMatcher.java:
##########
@@ -20,25 +20,25 @@
import java.io.IOException;
import java.util.ArrayList;
-import java.util.Collections;
import java.util.List;
-import org.apache.lucene.util.IntsRefBuilder;
+import java.util.function.IntConsumer;
import org.apache.lucene.util.automaton.Automaton;
import org.apache.lucene.util.automaton.CharacterRunAutomaton;
import org.apache.lucene.util.automaton.RegExp;
import org.apache.lucene.util.automaton.Transition;
import org.apache.lucene.util.fst.FST;
import org.apache.lucene.util.fst.Util;
+import org.apache.pinot.spi.query.QueryThreadContext;
-/**
- * RegexpMatcher is a helper to retrieve matching values for a given regexp
query.
- * Regexp query is converted into an automaton and we run the matching
algorithm on FST.
- *
- * Two main functions of this class are
- * regexMatchOnFST() Function runs matching on FST (See function comments
for more details)
- * match(input) Function builds the automaton and matches given input.
- */
+/// RegexpMatcher is a helper to retrieve matching values for a given regexp
query.
+/// Regexp query is converted into an automaton, and we run the matching
algorithm on FST.
+///
+/// Two main functions of this class are
+/// regexMatchOnFST() Function runs matching on FST (See function comments
for more details)
+/// match(input) Function builds the automaton and matches given input.
+///
+/// This class is not thread-safe. Create a new instance (or use the static
helper) per query.
Review Comment:
These `///` comments are not Javadoc and may break any Checkstyle/Javadoc
enforcement for public classes/methods, and they won’t show up in generated API
docs. Prefer converting public API documentation to proper Javadoc (`/** ...
*/`) (same applies to the new/updated public methods in this class and the
similar changes in `RegexpMatcherCaseInsensitive`,
`QueryOptionsUtils#getFstRegexpTraversalLimit`, and
`FSTTraversalLimitExceededException`).
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/fst/RegexpMatcher.java:
##########
@@ -48,9 +48,20 @@ public RegexpMatcher(String regexQuery, FST<Long> fst) {
_automaton = (new RegExp(regexQuery)).toAutomaton();
}
- public static List<Long> regexMatch(String regexQuery, FST<Long> fst)
+ /// Runs matching of the given regexp query on the FST, emitting the value
(dict id) of each matched entry to the
+ /// given consumer. Values are emitted as the traversal hits final states,
so the caller can collect them without
+ /// this class materializing the full result set in memory.
+ public static void regexMatch(String regexQuery, FST<Long> fst, IntConsumer
dictIdConsumer)
throws IOException {
- return new RegexpMatcher(regexQuery, fst).regexMatchOnFST();
+ regexMatch(regexQuery, fst, dictIdConsumer, Integer.MAX_VALUE);
+ }
Review Comment:
This changes the public API shape from returning a `List<Long>` to streaming
via `IntConsumer`, which is a breaking change for any external callers of
`RegexpMatcher` (and similarly `RegexpMatcherCaseInsensitive`). Consider
keeping the old signatures as deprecated convenience wrappers that collect into
a list internally (calling the new streaming overload), so downstream code
compiles while still benefiting from the new implementation.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/fst/RegexpMatcher.java:
##########
@@ -59,97 +70,90 @@ public boolean match(String input) {
return characterRunAutomaton.run(input);
}
- /**
- * This function runs matching on automaton built from regexQuery and the
FST.
- * FST stores key (string) to a value (Long). Both are state machines and
state transition is based on
- * a input character.
- *
- * This algorithm starts with Queue containing (Automaton Start Node, FST
Start Node).
- * Each step an entry is popped from the queue:
- * 1) if the automaton state is accept and the FST Node is final (i.e.
end node) then the value stored for that FST
- * is added to the set of result.
- * 2) Else next set of transitions on automaton are gathered and for each
transition target node for that character
- * is figured out in FST Node, resulting pair of (automaton state, fst
node) are added to the queue.
- * 3) This process is bound to complete since we are making progression
on the FST (which is a DAG) towards final
- * nodes.
- * @return
- * @throws IOException
- */
- public List<Long> regexMatchOnFST()
+ /// This function runs matching on automaton built from regexQuery and the
FST.
+ /// FST stores key (string) to a value (Long). Both are state machines and
state transition is based on an input
+ /// character.
+ ///
+ /// This algorithm starts with a stack containing (Automaton Start Node, FST
Start Node).
+ /// Each step an entry is popped from the stack (DFS order to bound the
frontier size):
+ /// 1) if the automaton state is accept and the FST Node is final (i.e.
end node) then the value stored for that
+ /// FST node is emitted to the consumer.
+ /// 2) Else next set of transitions on automaton are gathered and for each
transition target node for that
+ /// character is figured out in FST Node, resulting pair of (automaton
state, fst node) are added to the stack.
+ /// 3) This process is bound to complete since we are making progression
on the FST (which is a DAG) towards final
+ /// nodes.
+ ///
+ /// A broad regexp on a high-cardinality column can visit a huge number of
paths, so the loop periodically checks
+ /// for query termination (timeout or OOM-protection kill) to remain
interruptible.
+ public void regexMatchOnFST(IntConsumer dictIdConsumer)
+ throws IOException {
+ regexMatchOnFST(dictIdConsumer, Integer.MAX_VALUE);
+ }
+
+ /// Same as [#regexMatchOnFST(IntConsumer)] but throws
[FSTTraversalLimitExceededException] once `maxPaths` FST
+ /// paths have been visited.
+ public void regexMatchOnFST(IntConsumer dictIdConsumer, int maxPaths)
throws IOException {
- final List<Path<Long>> queue = new ArrayList<>();
- final List<Path<Long>> endNodes = new ArrayList<>();
if (_automaton.getNumStates() == 0) {
- return Collections.emptyList();
+ return;
}
- // Automaton start state and FST start node is added to the queue.
- queue.add(new Path<>(0, _fst.getFirstArc(new FST.Arc<Long>()),
_fst.outputs.getNoOutput(), new IntsRefBuilder()));
+ // Automaton start state and FST start node is added to the stack.
+ List<Path<Long>> stack = new ArrayList<>();
+ stack.add(new Path<>(0, _fst.getFirstArc(new FST.Arc<>()),
_fst.outputs.getNoOutput()));
- final FST.Arc<Long> scratchArc = new FST.Arc<>();
- final FST.BytesReader fstReader = _fst.getBytesReader();
+ FST.Arc<Long> scratchArc = new FST.Arc<>();
+ FST.BytesReader fstReader = _fst.getBytesReader();
Transition t = new Transition();
- while (!queue.isEmpty()) {
- final Path<Long> path = queue.remove(queue.size() - 1);
-
- // If automaton is in accept state and the fstNode is final (i.e. end
node) then add the entry to endNodes which
- // contains the result set.
- if (_automaton.isAccept(path._state)) {
- if (path._fstNode.isFinal()) {
- endNodes.add(path);
- }
+ int numPathsProcessed = 0;
+ while (!stack.isEmpty()) {
+ if (numPathsProcessed >= maxPaths) {
+ throw new FSTTraversalLimitExceededException(
+ "FST regexp traversal exceeded the limit of " + maxPaths + "
paths");
+ }
+
QueryThreadContext.checkTerminationAndSampleUsagePeriodically(numPathsProcessed++,
+ "RegexpMatcher#regexMatchOnFST");
Review Comment:
`maxPaths <= 0` currently causes an immediate throw on the first loop
iteration (since `0 >= maxPaths`), which is surprising given the query option
semantics described elsewhere (‘non-positive disables the cap’). To align
behavior and avoid foot-guns for direct callers of these public methods, treat
`maxPaths <= 0` as unbounded (e.g., normalize to `Integer.MAX_VALUE`) in
`regexMatch(...)`/`regexMatchOnFST(...)` for both matchers, or alternatively
validate and throw an `IllegalArgumentException` with a clear message.
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/config/QueryOptionsUtils.java:
##########
@@ -711,6 +711,15 @@ public static Integer
getRegexDictSizeThreshold(Map<String, String> queryOptions
return uncheckedParseInt(QueryOptionKey.REGEX_DICT_SIZE_THRESHOLD,
regexDictSizeThreshold);
}
+ /// Cap on the number of FST paths visited per FST/IFST-backed REGEXP_LIKE
evaluation before falling back to the
+ /// dictionary-scan evaluator. Returns `null` if the query option is not
set, in which case
+ ///
[CommonConstants.Broker.Request.QueryOptionValue#DEFAULT_FST_REGEXP_TRAVERSAL_LIMIT]
applies.
+ @Nullable
+ public static Integer getFstRegexpTraversalLimit(Map<String, String>
queryOptions) {
+ String fstRegexpTraversalLimit =
queryOptions.get(QueryOptionKey.FST_REGEXP_TRAVERSAL_LIMIT);
+ return uncheckedParseInt(QueryOptionKey.FST_REGEXP_TRAVERSAL_LIMIT,
fstRegexpTraversalLimit);
+ }
Review Comment:
This is a public method but uses `///` instead of Javadoc. If the project
enforces Javadoc on public APIs, this can fail style checks and won’t appear in
generated docs; please convert to `/** ... */` Javadoc (and consider using
`{@link ...}` instead of bracket-style references).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
