mikemccand commented on code in PR #12624:
URL: https://github.com/apache/lucene/pull/12624#discussion_r1350191783
##########
lucene/core/src/java/org/apache/lucene/util/fst/BytesStore.java:
##########
@@ -21,19 +21,18 @@
import java.util.List;
import org.apache.lucene.store.DataInput;
import org.apache.lucene.store.DataOutput;
-import org.apache.lucene.util.Accountable;
import org.apache.lucene.util.RamUsageEstimator;
// TODO: merge with PagedBytes, except PagedBytes doesn't
// let you read while writing which FST needs
-class BytesStore extends DataOutput implements Accountable {
+class BytesStore extends FSTWriter {
Review Comment:
I'm a little nervous about this approach -- I was hoping to reduce the
storage-like abstractions here, by sticking with `DataOutput` and showing how
it could be backed by an `IndexOutput` that streams straight to disk. But this
PR is adding yet another storage abstraction :)
Maybe a simpler first baby step would be to eliminate APIs in `BytesStore`
that an `IndexOutput` would not easily support, e.g. this sneaky `reverse`
method? It is only called in one place, to reverse just written bytes, so that
place could/should just as easily buffer up its own `byte[]` in a
scratch/reused array, reverse that in place before writing, then write it?
Then we can remove this `reverse` method and `BytesStore` is closer to just a
`DataOutput`?
--
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
