Move addNode to FSTCompiler #12646
Move addNode to FSTCompiler #12646
Conversation
|
Thanks for opening @dungba88! This FST building code is very hairy and this is a nice start at cleaning it up. Given how expert this code is and that the relevant methods are all package-private I don't see a problem with backporting this to 9x - what do you think @mikemccand? |
There was a problem hiding this comment.
This is a great cleanup! I just had one question about a newly public method.
Thanks @dungba88!
| /** | ||
| * Gets the number of bytes required to flag the presence of each arc in the given label range, | ||
| * one bit per arc. | ||
| */ | ||
| private static int getNumPresenceBytes(int labelRange) { | ||
| public static int getNumPresenceBytes(int labelRange) { |
There was a problem hiding this comment.
Can we make this package private instead (remove public)?
There was a problem hiding this comment.
Ah good catch! I have removed it
+1 to backport to 9x -- this is nice refactoring that does not change any API and is low risk. In general, we should strive to backport all changes to 9.x by default unless there is a solid reason not to (e.g. unavoidable / complex API break). Thanks @dungba88 and @romseygeek! |
I can do this. Wondering if creating a PR to lucene-9_x branch would suffice? |
|
I'm happy to merge and backport @dungba88. Can you also add an entry to CHANGES.txt in the 9.9 section? |
|
Thanks @romseygeek I have added the entry (under API change section). |
|
Thanks @dungba88! |
Currently FSTCompiler and FST have circular dependencies to each other. FSTCompiler creates an instance of FST, and on adding node (add(IntsRef input, T output)), it delegates to FST.addNode() and passes itself as a variable. This introduces a circular dependency and mixes up the FST constructing and traversing code. To make matter worse, this implies one can call FST.addNode with an arbitrary FSTCompiler (as it's a parameter), but in reality it should be the compiler which creates the FST. This commit moves the addNode method to FSTCompiler instead. Co-authored-by: Anh Dung Bui <buidun@amazon.com>
Description
Currently FSTCompiler and FST has a circular dependencies to each other. FSTCompiler creates an instance of FST, and on adding node (
add(IntsRef input, T output)), it delegates toFST.addNode()and passing itself as a variable. This introduces a circular dependency and mixes up the FST constructing and traversing code.To make matter worse, this implies one can call
FST.addNodewith an arbitrary FSTCompiler (as it's a parameter), but in reality it should be the compiler which creates the FST.This PR move the
addNodemethod to FSTCompiler instead.