mikemccand commented on PR #12286: URL: https://github.com/apache/lucene/pull/12286#issuecomment-1543997539
Thank you @tang-hi! We have tried to build iterative versions of this in the past, but it was conterversial because it was so much more complicated than the existing recursive version. But your iterative solution looks very simple! > However, I noticed that the test [TestAutomaton.java](https://github.com/apache/lucene/blob/963ed7ce888724c2dd55fff8c13a08b81b20f535/lucene/core/src/test/org/apache/lucene/util/automaton/TestAutomaton.java#LL1205C7-L1205C7) depends on the order in which recursive calls are made or items are added to and removed from the stack. The test really should not be relying on such an implementation detail? By definition, toposort might have many valid outputs for the same input graph. The test should only assert that the returned order indeed ensures that all transitions are only forwards in the sorted states. So I think it's fine to fix this test bug and keep the simpler iterative solution here? > 3. Should we continue to limit the size of the Automaton? I don't think we should. > 2. Perhaps we should check if the automaton contains cycles, and throw an `IllegalArgumentException` if it does? That would be nice, though it really is illegal usage of the API -- can you detect that w/o much added cost in the iterative solution? -- 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: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
