You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "javanna (via GitHub)" <gi...@apache.org> on 2025/02/21 14:22:14 UTC
[PR] fix [lucene]
javanna opened a new pull request, #14270:
URL: https://github.com/apache/lucene/pull/14270
We currently have a gap in testing the completion postings format. While it allows to load FSTs off heap, and the format has a constructor that takes the fst load mode, at read time SPI goes through the default constructor that has fst load mode hardcoded to on heap at all times. This means that despite we randomize fst load mode in `TestSuggestField`, we never effectively load the completion FST off heap, we only set the load mode to something different than on heap when writing, where the load mode takes no effect.
This commit introduces a static method that allows to switch the loading of the FST for all the completion postings format.
This unveils a bug when loading FST off heap and the index input is a `MemorySegmentIndexInput` which I am going a PR for separately.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Address completion fields testing gap and truly allow loading FST off heap [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna closed pull request #14270: Address completion fields testing gap and truly allow loading FST off heap
URL: https://github.com/apache/lucene/pull/14270
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] fix [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #14270:
URL: https://github.com/apache/lucene/pull/14270#discussion_r1965565789
##########
lucene/suggest/src/java/org/apache/lucene/search/suggest/document/CompletionPostingsFormat.java:
##########
@@ -122,18 +125,13 @@ public enum FSTLoadMode {
private final FSTLoadMode fstLoadMode;
- /** Used only by core Lucene at read-time via Service Provider instantiation */
- public CompletionPostingsFormat(String name) {
- this(name, FSTLoadMode.ON_HEAP);
- }
Review Comment:
I believe the comment here is incorrect, this constructor was unused and is redundant.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] fix [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #14270:
URL: https://github.com/apache/lucene/pull/14270#discussion_r1965567430
##########
lucene/suggest/src/test/org/apache/lucene/search/suggest/document/TestSuggestField.java:
##########
@@ -949,10 +952,7 @@ static IndexWriterConfig iwcWithSuggestField(Analyzer analyzer, final Set<String
iwc.setMergePolicy(newLogMergePolicy());
Codec filterCodec =
new FilterCodec(TestUtil.getDefaultCodec().getName(), TestUtil.getDefaultCodec()) {
- final CompletionPostingsFormat.FSTLoadMode fstLoadMode =
- RandomPicks.randomFrom(random(), CompletionPostingsFormat.FSTLoadMode.values());
- final PostingsFormat postingsFormat = new Completion101PostingsFormat(fstLoadMode);
Review Comment:
While this looked like randomizing the load mode, it did not as SPI would not go through this constructor. Setting fst load mode this way would take no effect.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] fix [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #14270:
URL: https://github.com/apache/lucene/pull/14270#discussion_r1965564752
##########
lucene/suggest/src/java/org/apache/lucene/search/suggest/document/CompletionPostingsFormat.java:
##########
@@ -104,6 +104,9 @@ public abstract class CompletionPostingsFormat extends PostingsFormat {
static final String INDEX_EXTENSION = "cmp";
static final String DICT_EXTENSION = "lkp";
+ public static final FSTLoadMode DEFAULT_FST_LOAD_MODE = FSTLoadMode.ON_HEAP;
+ public static FSTLoadMode FST_LOAD_MODE = DEFAULT_FST_LOAD_MODE;
Review Comment:
A mutable static field is certainly not elegant. I am out of ideas of how we can allow to swap reading between on_heap and off_heap without requiring a specific postings format for each load mode, which would be odd given loading is merely a read aspect and does not affect the writing side in any way. Having to rely on the default constructor is quite limiting. On the other hand this is an improvement compared to what we had until now.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Address completion fields testing gap and truly allow loading FST off heap [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #14270:
URL: https://github.com/apache/lucene/pull/14270#issuecomment-2678961874
@jpountz do you have opinions on this? Who else should I ping otherwise?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Address completion fields testing gap and truly allow loading FST off heap [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #14270:
URL: https://github.com/apache/lucene/pull/14270#issuecomment-2687334196
That's good with me. Shall we clearly document this then as a follow-up, and shall I make the fst load mode static field package private perhaps, so that we at least test both modes?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Address completion fields testing gap and truly allow loading FST off heap [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on PR #14270:
URL: https://github.com/apache/lucene/pull/14270#issuecomment-2686199835
> I personally find it not so user-friendly to require users to define their own codec only for loading purposes, when there is no effect on how stuff is written. Is that a common thing users may do in other scenarios like inverted indices, doc values etc. ?
The way I'm seeing it: for other data structures like inverted indexes and doc values, Lucene takes full ownership of what should live on-heap (terms index, doc values metadata, etc.) and what should live off-heap (terms, postings, etc.). Users don't have a way to configure it. I would like `completion` to be the same: Lucene makes a decision of whether this FST should be on-heap or off-heap once for all, and removes the ability to tune whether to load it on-heap or off-heap.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Address completion fields testing gap and truly allow loading FST off heap [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #14270:
URL: https://github.com/apache/lucene/pull/14270#issuecomment-2722992549
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Address completion fields testing gap and truly allow loading FST off heap [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #14270:
URL: https://github.com/apache/lucene/pull/14270#issuecomment-2685272175
I think that we do we need to have a way to truly make the switch at least for testing purposes, otherwise we only test one approach which is error prone. That's the original issue that this PR addresses
From a users perspective, I don't have data at hand to show that off-heap is as performant as on heap. Given how difficult it's been to use off heap completion fields, I'd assume that everyone always relied on on-heap, as that has been the default since day one years ago. I think exposing a way to make the switch would be handy.
I personally find it not so user-friendly to require users to define their own codec only for loading purposes, when there is no effect on how stuff is written. Is that a common thing users may do in other scenarios like inverted indices, doc values etc. ?
If you are suggesting switching to off-heap by default, I am +1 for that.
FYI, another problem we have around providing custom completion postings formats is going to be addressed by #14275, so that existing SPI services can be predictably replaced. Once that is fixed, either changing the default or requiring users to provide their own postings format to override the default behaviour is probably good enough. In that case I would leave the static handling I am adding in this PR only for testing purposes.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Address completion fields testing gap and truly allow loading FST off heap [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #14270:
URL: https://github.com/apache/lucene/pull/14270#issuecomment-2729966315
I opened #14364 to still address the testing gap, but also change the default load mode to off heap.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Address completion fields testing gap and truly allow loading FST off heap [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on PR #14270:
URL: https://github.com/apache/lucene/pull/14270#issuecomment-2685149108
Do we need to let users configure whether to load their completion fields on-heap or off-heap? Our inverted indexes, doc values, etc. don't give the option - if users want to load data on-heap, they need to create their own codec. Could it be the same with `CompletionPostingsFormat`? Plus who wants to load GBs of data on heap?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] fix [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #14270:
URL: https://github.com/apache/lucene/pull/14270#discussion_r1965557931
##########
lucene/suggest/src/java/org/apache/lucene/search/suggest/document/Completion101PostingsFormat.java:
##########
@@ -25,17 +25,10 @@
* @lucene.experimental
*/
public class Completion101PostingsFormat extends CompletionPostingsFormat {
- /** Creates a {@link Completion101PostingsFormat} that will load the completion FST on-heap. */
- public Completion101PostingsFormat() {
- this(FSTLoadMode.ON_HEAP);
Review Comment:
This is the constructor called by SPI when reading, which has on heap as hardcoded load mode.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] fix [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on code in PR #14270:
URL: https://github.com/apache/lucene/pull/14270#discussion_r1965557931
##########
lucene/suggest/src/java/org/apache/lucene/search/suggest/document/Completion101PostingsFormat.java:
##########
@@ -25,17 +25,10 @@
* @lucene.experimental
*/
public class Completion101PostingsFormat extends CompletionPostingsFormat {
- /** Creates a {@link Completion101PostingsFormat} that will load the completion FST on-heap. */
- public Completion101PostingsFormat() {
- this(FSTLoadMode.ON_HEAP);
Review Comment:
This is the constructor called by SPI when reading, and it has on heap hardcoded.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org