Add BytesRefIterator to TermInSetQuery

[cbuescher]

Addresses #13804

TermInSetQuery used to have an accessor to its terms that was removed in #12173 to protect leaking internal encoding details. This introduces an accessor to the term data in the query that doesn't expose internal but merely allows iterating over the decoded BytesRef, making inspection of the querys content possible again.

[rmuir]

good solution. could we consider also fixing the visitor to use this approach (vs passing a RunAutomaton or something awful?)

[rmuir]

and the question is not for this PR, just a general one. It seems the only "real user" of consumeTermsMatching is highlighter, and building an automaton for this thing seems to be... both very heavy and awkward? Just for a future issue/followup...

[rmuir]

maybe add javadocs for the method and @experimental for now?

its possible we could "fix visitor" api in the future where we wouldn't need this public method specific to this query as well (see general thoughts on PR)

[cbuescher]

Sure, thanks.

[rmuir]

also, it would be good to get an idea of the use-case. The problem is, this query can hold many terms:

• getting an automaton over them isn't really a use-case, highlighting docs is. We should make this more efficient. This existing visitor api seems way too prescriptive: probably shouldn't be Supplier<ByteRunAutomaton> automaton but should probably just be Predicate<BytesRef> which supports composition with and/or/etc and also supports matching terms with test(BytesRef). This is more general API (even if it doesn't help us with PrefixCodedTerms immediately).
• for similar reasons, getting an iterator over ALL terms isn't a use-case... its a slow linear-time implementation of whatever you are doing. So it would be great to get an idea of what that is, so that we can get to a good interface eventually.

[javanna]

Thanks for taking a look @rmuir !

I have been digging a bit through history, it seems like it used to be possible to get all the terms via QueryVisitor#consumeTerms, but that changed with 267d70b to build an automaton instead. That effectively removed the ability to get the actual terms from TermInSetQuery via query visitor. Later, getTermData was also removed from TermInSetQuery, as it exposed internal encoding of the terms. I agree that we should look at how to make the API better in the long run.

The main usecase we have is monitor alike, where you have queries stored in the index, and want to run them against incoming documents. In order to pre-filter the queries and reduce the amount of them that we need to run, we extract terms from them upon indexing and put them in a separate field that we later use to apply pre-filtering. I checked in the monitor code and it looks like its query visitor (from QueryAnalyzer) ignores this case, hence TermInSetQuery with more than one term will not allow to perform pre-filtering, which means that more queries will be potential candidates that could be excluded ahead of time if we were able to extract terms from them. I am not sure that the predicate idea would work for this scenario, I don't think we can move away from building a list of terms for this usecase?

With that, I am going to go ahead and merge this PR for now to main, that unblocks us for now, and we can continue the discussion about the long term approach.

[javanna]

@cbuescher could you add an entry to CHANGES.txt, under 9.12 please? I am thinking that this should be backported so it provides a replacement for the deprecated method before it gets removed. I can take care of adding a link to it in the javadocs of the deprecated term in 9.x when I do the backporting.

[cbuescher]

@javanna done, thanks.

[javanna]

LGTM