mikemccand commented on issue #16392: URL: https://github.com/apache/lucene/issues/16392#issuecomment-4980164830
> > I'm confused -- segment name is always available in `SegmentReadState`, at the lowest level (under `SegmentInfo.name`). Each low level reader uses that to construct the file names it then opens. I think we should keep things simple and key by the String fileName (what the Codec component passes to `Directory.openInput`)? > > In the spirit of keeping the changes minimal I did not want to force codecs to maintain references to the `SegmentInfo`/file names that otherwise seem to be referenced only during construction. To that extent, I had envisioned leaf readers just describing via file extension (`vec`, `vex`, `pos`, etc), then the wrapping readers prepending information accordingly (e.g. per-field codecs add to the file name). Finally `SegmentReader` would be able to complete the file names with the `SegmentInfo` it already references. It's fine to expect Codec readers to know the filenames they opened: the whole purpose of each Codec reader is to read just the one segment, and`SegmentReadState` already carefully provides all necessary context so each reader can easily compute its exact filenames (setting the right suffix for per-field embedded readers). Let's not re-invent that same mechanism in reverse (going back up the call stack). > > this is a per-segment thing, but what does "home" mean? Is it where this API is exposed? > > Yes, I mean where the API is exposed at the top level. You could argue for making `IndexReader` itself `DiskAccountable` too (aggregating over its leaves), but as you mentioned, consumers likely want to cache per-segment layouts using `CoreCacheHelper`. At the `IndexReader` level the segment geometry would be lost unless it caches internally itself, which is arguably too invasive. Maybe at least we could here provide the sugar for caching? It'd be a small wrapper class, using Lucene's `CoreCacheHelper` so things are evicted when the leaf is finally closed. > Is there appetite for modeling a range shared by multiple fields (e.g. deduplicated vectors), or is that better left out until a concrete need exists? +1 to design for today (postpone appetite for a future apple pie). Let's keep it simple --- phase one with no CFS, single field per extent -- that's hard enough! We already see that per-field accounting for stored fields isn't really possible. This new mechanism cannot be perfect. It's sort of like [Gödel's incompleteness theorems](https://en.wikipedia.org/wiki/G%C3%B6del%27s_incompleteness_theorems) heh. Let's target incompleteness rather than inconsistent. > Should the base API lean toward a visitor/callback style (accumulate into a caller-supplied sink, allocating nothing) rather than materializing an actual Map<String, List<Extent>>? This is an interesting question. If the underlying API is a visitor (no added GC load), we could implement the full `List<Extent>` as a wrapper/layer overlay on top. Visitor model might more naturally be extended over time too, e.g. we could add method for multiple fields per extent and by default fall back to single field for back compat. And, we could provide sugar for basic use cases e.g. a visitor that tallies up total disk usage by field. That would work well I think. Now I'm also wondering why we need "structural" and "unaccounted" types as well -- why insist the entire file is accounted for? If the default impl will fill in ranges for "unaccounted", it seems pointless since caller can equally well see such extents as unaccounted. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
