mikemccand commented on code in PR #12709:
URL: https://github.com/apache/lucene/pull/12709#discussion_r1369002933
##########
lucene/core/src/java/org/apache/lucene/util/fst/FST.java:
##########
@@ -487,19 +473,18 @@ public String toString() {
}
void finish(long newStartNode) throws IOException {
- assert newStartNode <= bytes.getPosition();
+ assert newStartNode <= fstReader.size();
if (startNode != -1) {
throw new IllegalStateException("already finished");
}
if (newStartNode == FINAL_END_NODE && emptyOutput != null) {
newStartNode = 0;
}
startNode = newStartNode;
- bytes.finish();
Review Comment:
Hmm was/is this a no-op?
Edit: nevermind -- I see you moved it up in the call stack
(`FSTCompiler.compile`).
##########
lucene/core/src/java/org/apache/lucene/util/fst/FSTCompiler.java:
##########
@@ -137,9 +137,11 @@ private FSTCompiler(
float directAddressingMaxOversizingFactor) {
this.allowFixedLengthArcs = allowFixedLengthArcs;
this.directAddressingMaxOversizingFactor =
directAddressingMaxOversizingFactor;
- fst = new FST<>(inputType, outputs, bytesPageBits);
- bytes = fst.bytes;
- assert bytes != null;
+ bytes = new BytesStore(bytesPageBits);
+ // pad: ensure no node gets address 0 which is reserved to mean
+ // the stop state w/ no arcs
+ bytes.writeByte((byte) 0);
Review Comment:
Excellent to move this out of `FST` to here, making it more consistent that
it's the `FSTCompiler` that does the writing, and `FST` that does the reading.
##########
lucene/core/src/java/org/apache/lucene/util/fst/FSTCompiler.java:
##########
@@ -317,8 +319,6 @@ private CompiledNode compileNode(UnCompiledNode<T> nodeIn,
int tailLength) throw
// serializes new node by appending its bytes to the end
// of the current byte[]
long addNode(FSTCompiler.UnCompiledNode<T> nodeIn) throws IOException {
- T NO_OUTPUT = fst.outputs.getNoOutput();
Review Comment:
This was dead? I wonder why nothing in our build statically finds our dead
code...
##########
lucene/core/src/java/org/apache/lucene/util/fst/FSTCompiler.java:
##########
@@ -859,6 +859,7 @@ public FST<T> compile() throws IOException {
// if (DEBUG) System.out.println(" builder.finish root.isFinal=" +
root.isFinal + "
// root.output=" + root.output);
fst.finish(compileNode(root, lastInput.length()).node);
+ bytes.finish();
Review Comment:
Oh I see, you just moved the `.finish()` to here, OK.
##########
lucene/core/src/java/org/apache/lucene/util/fst/FST.java:
##########
@@ -555,16 +540,8 @@ public void save(DataOutput metaOut, DataOutput out)
throws IOException {
}
metaOut.writeByte(t);
metaOut.writeVLong(startNode);
- if (bytes != null) {
- long numBytes = bytes.getPosition();
- metaOut.writeVLong(numBytes);
- bytes.writeTo(out);
- } else {
- assert fstStore != null;
- long numBytes = fstStore.size();
- metaOut.writeVLong(numBytes);
- fstStore.writeTo(out);
- }
+ metaOut.writeVLong(numBytes());
Review Comment:
So much cleaner, I love it! No more scattered ifs depending on which store
is backing the FST...
--
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
