fix(newmm): resolve exponential BFS path explosion in ambiguous tokenization - #101
Conversation
- Add visited set to bfs_paths_graph to prevent re-exploring already-visited nodes, reducing worst-case BFS from O(2^n) to O(V+E) - Clear ambiguity graph after each commit point in one_cut to prevent unbounded edge accumulation - Remove unnecessary graph edge insertion in the no-candidate branch - Add test_newmm_ambiguous_performance regression test - Update CHANGELOG.md Agent-Logs-Url: https://github.com/PyThaiNLP/nlpo3/sessions/b1879073-e5b6-44a8-8492-7eb13a0cb26c Co-authored-by: bact <128572+bact@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a worst-case exponential blow-up in Newmm’s BFS-based ambiguity resolution by ensuring graph traversal doesn’t repeatedly re-enqueue already-seen nodes, and by clearing the ambiguity graph at commit points to avoid stale edges accumulating across segments.
Changes:
- Add a
visitedset tobfs_paths_graphto prevent repeated exploration of the same positions. - Clear the ambiguity graph (
graph.clear()) when committing a resolved segment and when handling the no-candidate branch. - Add a regression test that exercises highly ambiguous input and asserts it completes within a time bound.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/tokenizer/newmm.rs |
Prevents BFS queue explosion via visited tracking and clears graph state between committed segments. |
tests/test_tokenizer.rs |
Adds a regression test for highly ambiguous input to guard against BFS path explosion. |
CHANGELOG.md |
Documents the Newmm BFS/graph fixes and the newly added regression test. |
|
@copilot apply changes based on the comments in this thread There are questions about claims on reduce complexity don't match the actual implementation. Recheck the implementation if anything is left unoptimized, or the claim is inaccurate. |
- Fix doc comment: _bfs_paths_graph -> bfs_paths_graph in test - Fix spelling: maximising -> maximizing (American English) - Reword BFS comment: drop inaccurate O(V+E) claim; say each vertex is enqueued at most once, avoiding exponential blow-up - Update CHANGELOG: replace O(V+E) claim with accurate description of avoiding exponential queue growth Agent-Logs-Url: https://github.com/PyThaiNLP/nlpo3/sessions/b8052556-3598-419d-80e8-7af979d714e3 Co-authored-by: bact <128572+bact@users.noreply.github.com>
Applied all four review suggestions in commit
|
The newmm BFS graph traversal had O(2^n) worst-case behavior on highly ambiguous input — the same bug fixed in Python via PyThaiNLP/pythainlp#1319. Without a visited set, every neighbor is unconditionally re-enqueued, causing the queue to grow exponentially when the dictionary produces many overlapping matches.
Changes
bfs_paths_graph— addvisited: HashSet<CharacterIndex>: Only enqueue nodes not yet seen. Reduces worst-case from O(2^n) to O(V+E).one_cutcommit-point branch — callgraph.clear(): Previously onlygraph_sizewas reset to 0; stale edges from resolved segments accumulated across BFS calls. Graph is now cleared at each commit point.one_cutno-candidate branch — remove unnecessary edge insertion: The non-dictionary-match branch inserted a graph edge that was never needed (token is yielded directly). Replaced withgraph_size = 0; graph.clear(), consistent with the commit-point branch.Regression test (
test_newmm_ambiguous_performance): 250-character maximally ambiguous input (1–3 char overlapping words covering every TCC boundary) must complete within 1 s. With the fix it runs in ~10 ms.