You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "cbuescher (via GitHub)" <gi...@apache.org> on 2024/09/18 21:23:38 UTC
[PR] Add BytesRefIterator to TermInSetQuery [lucene]
cbuescher opened a new pull request, #13806:
URL: https://github.com/apache/lucene/pull/13806
Addresses #13778
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.
--
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] Add BytesRefIterator to TermInSetQuery [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on PR #13806:
URL: https://github.com/apache/lucene/pull/13806#issuecomment-2359438720
good solution. could we consider also fixing the visitor to use this approach (vs passing a RunAutomaton or something awful?)
--
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] Add BytesRefIterator to TermInSetQuery [lucene]
Posted by "cbuescher (via GitHub)" <gi...@apache.org>.
cbuescher commented on code in PR #13806:
URL: https://github.com/apache/lucene/pull/13806#discussion_r1766408644
##########
lucene/core/src/java/org/apache/lucene/search/TermInSetQuery.java:
##########
@@ -141,6 +135,11 @@ public long getTermsCount() {
return termData.size();
}
+ public BytesRefIterator getBytesRefIterator() {
Review Comment:
Sure, thanks.
--
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] Add BytesRefIterator to TermInSetQuery [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna merged PR #13806:
URL: https://github.com/apache/lucene/pull/13806
--
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] Add BytesRefIterator to TermInSetQuery [lucene]
Posted by "cbuescher (via GitHub)" <gi...@apache.org>.
cbuescher commented on PR #13806:
URL: https://github.com/apache/lucene/pull/13806#issuecomment-2360483799
@javanna done, thanks.
--
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] Add BytesRefIterator to TermInSetQuery [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on code in PR #13806:
URL: https://github.com/apache/lucene/pull/13806#discussion_r1765753269
##########
lucene/core/src/java/org/apache/lucene/search/TermInSetQuery.java:
##########
@@ -141,6 +135,11 @@ public long getTermsCount() {
return termData.size();
}
+ public BytesRefIterator getBytesRefIterator() {
Review Comment:
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)
--
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] Add BytesRefIterator to TermInSetQuery [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #13806:
URL: https://github.com/apache/lucene/pull/13806#issuecomment-2360398761
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 https://github.com/apache/lucene/commit/267d70b66b6ac30db1d48f131b294512420f468c 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.
--
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] Add BytesRefIterator to TermInSetQuery [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #13806:
URL: https://github.com/apache/lucene/pull/13806#issuecomment-2360422815
@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.
--
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] Add BytesRefIterator to TermInSetQuery [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on PR #13806:
URL: https://github.com/apache/lucene/pull/13806#issuecomment-2359446319
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...
--
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] Add BytesRefIterator to TermInSetQuery [lucene]
Posted by "rmuir (via GitHub)" <gi...@apache.org>.
rmuir commented on PR #13806:
URL: https://github.com/apache/lucene/pull/13806#issuecomment-2359564950
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.
--
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