Fix: visit of inner query for FunctionScoreQueryBuilder #15404
Conversation
jdnvn
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
dblock,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami and
VachaShah
as code owners
|
❕ Gradle check result for d8e8390: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15404 +/- ##
============================================
- Coverage 71.95% 71.92% -0.03%
+ Complexity 63254 63229 -25
============================================
Files 5224 5224
Lines 296079 296083 +4
Branches 42763 42764 +1
============================================
- Hits 213030 212946 -84
- Misses 65550 65671 +121
+ Partials 17499 17466 -33 ☔ View full report in Codecov by Sentry. |
Signed-off-by: jdnvn <[email protected]>
Signed-off-by: jdnvn <[email protected]>
|
❌ Gradle check result for 3258eea: 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? |
|
@yuye-aws @zhichao-aws @vibrantvarun hello, I updated the PR and CI is green now. Mind taking another look? |
| public void visit(QueryBuilderVisitor visitor) { | ||
| visitor.accept(this); | ||
| if (query != null) { | ||
| visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(query); | ||
| } |
There was a problem hiding this comment.
Nitpick -- it looks like the FunctionScoreQueryBuilder constructors guarantee that query can never be null, so the check is unnecessary.
There was a problem hiding this comment.
A similar comment was made here, and the author opted to keep the null check to pass tests and keep consistent with the other implementations. Let me know and I can change it if you feel otherwise.
There was a problem hiding this comment.
I think for unit test case purpose (Keeping method isolated) a null check would be fine
...r/src/test/java/org/opensearch/index/query/functionscore/FunctionScoreQueryBuilderTests.java
Show resolved
Hide resolved
Signed-off-by: Joe Donovan <[email protected]>
|
❌ Gradle check result for 39b50b9: 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 378b984: 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? |
|
@jdnvn Can you fix the DCO and gradle check? |
|
This PR is stalled because it has been open for 30 days with no activity. |
|
@msfroh Can you rerun the gradle checks? |
|
@jdnvn It looks like one of your commits is unsigned. That's breaking the DCO check. You can |
|
Since we've had at least one release since this PR was opened, we'll need to rebase it onto the latest |
Description
Similar to #14739, but implements the visit method in FunctionScoreQueryBuilder to allow subqueries to be processed properly.
Related Issues
Resolves #15403
Similar to #15034
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.