-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Enhance terms lookup query to take a query clause #18195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Enhance terms lookup query to take a query clause #18195
Conversation
❌ Gradle check result for 085edf2: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java
Outdated
Show resolved
Hide resolved
❌ Gradle check result for edfa605: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for f7146ff: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for 90f90c7: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for 997ddbe: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for 9c85868: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
❌ Gradle check result for 6f52602: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
da0ba03
to
964f452
Compare
…use rather than docId Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
…checks and updated unit tests Signed-off-by: Srikanth Padakanti <[email protected]>
…checks and updated unit tests Signed-off-by: Srikanth Padakanti <[email protected]>
…checks and updated unit tests Signed-off-by: Srikanth Padakanti <[email protected]>
…checks and updated unit tests Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
964f452
to
6ab5410
Compare
❌ Gradle check result for 6ab5410: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Srikanth Padakanti <[email protected]>
❌ Gradle check result for 101c8ad: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Srikanth Padakanti <[email protected]>
@kotwanikunal @Bukhtawar @andrross @sandeshkr419 |
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
❌ Gradle check result for ab7abdb: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
||
public String id() { | ||
return id; | ||
out.writeOptionalWriteable(query); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add in a version check here as well
out.writeOptionalWriteable(query); | |
if (out.getVersion().onOrAfter(Version.V_3_0_0)) { | |
out.writeOptionalWriteable(query); | |
} |
@@ -90,6 +114,11 @@ public TermsLookup(StreamInput in) throws IOException { | |||
if (in.getVersion().onOrAfter(Version.V_2_17_0)) { | |||
store = in.readBoolean(); | |||
} | |||
if (in.getVersion().onOrAfter(Version.V_3_0_0)) { | |||
if (in.readBoolean()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you should be doing the in.readBoolean()
explicitly.
ReadOptionalWriteable should take care of it for you.
Reference:
public <T extends Writeable> T readOptionalWriteable(Writeable.Reader<T> reader) throws IOException {
if (readBoolean()) {
T t = reader.read(this);
if (t == null) {
throw new IOException(
"Writeable.Reader [" + reader + "] returned null which is not allowed and probably means it screwed up the stream."
);
}
return t;
} else {
return null;
}
}
SearchResponse response; | ||
try { | ||
response = shardContext.getClient() | ||
.search( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if there is a better location for fetching rather than rewrites. If I understand correctly, rewrite can be called multiple times through the query flow. We would have wasted search calls.
The actionGet()
is also blocking the search thread on this query for a network call - which is not ideal.
Can we initialize this value at the start of the query creation and keep using the same value as a class var?
Also, can you please extract the query specific mechanism into a separate method for easy readability?
if (termsLookup != null && termsLookup.query() != null) { | ||
QueryBuilder rewrittenQuery = termsLookup.query().rewrite(context); | ||
|
||
SearchResponse response = context.getClient() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we store this value and reuse it for rewrite phases?
Also can we avoid blocking calls on the search thread?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this will need to be asynchronous, probably similar to how the current terms lookup query is implemented:
OpenSearch/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java
Lines 609 to 612 in 52b8b05
queryRewriteContext.registerAsyncAction((client, listener) -> fetch(termsLookup, client, ActionListener.map(listener, list -> { | |
supplier.set(list); | |
return null; | |
}))); |
@@ -6,6 +6,10 @@ | |||
<option name="HOST" value="localhost" /> | |||
<option name="PORT" value="5005" /> | |||
<option name="AUTO_RESTART" value="true" /> | |||
<RunnerSettings RunnerId="Debug"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change unless this is intentional.
if (termsLookup != null && termsLookup.query() != null) { | ||
QueryBuilder rewrittenQuery = termsLookup.query().rewrite(context); | ||
|
||
SearchResponse response = context.getClient() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this will need to be asynchronous, probably similar to how the current terms lookup query is implemented:
OpenSearch/server/src/main/java/org/opensearch/index/query/TermsQueryBuilder.java
Lines 609 to 612 in 52b8b05
queryRewriteContext.registerAsyncAction((client, listener) -> fetch(termsLookup, client, ActionListener.map(listener, list -> { | |
supplier.set(list); | |
return null; | |
}))); |
This PR is stalled because it has been open for 30 days with no activity. |
@srikanthpadakanti are you planning to continue working on this? We are targeting this feature for 3.2 release |
Description
Enhances the Terms lookup query to accept a query clause instead of only supporting a single docId. This expands functionality by enabling users to define document matches through flexible query logic rather than relying solely on fixed document IDs.
This change includes:
Support for Terms lookup query to use a query clause.
Validation logic to prevent using both id and query simultaneously.
Associated unit test updates to cover query-based lookup behavior.
Related Issues
Resolves #17599
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.