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. |
|
Hi @gaobinlong and @zhichao-aws ! Can you help review this PR? |
|
I'd recommend @vibrantvarun take a look at this PR, thanks! |
|
Hi @jdnvn ! The DCO checks have failed. Can you follow the guide https://github.com/opensearch-project/OpenSearch/pull/15404/checks?check_run_id=29224905792 to fix this? |
Signed-off-by: jdnvn <joe@donovan.to>
Signed-off-by: jdnvn <joe@donovan.to>
|
❌ 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.
...r/src/test/java/org/opensearch/index/query/functionscore/FunctionScoreQueryBuilderTests.java
Show resolved
Hide resolved
Signed-off-by: Joe Donovan <jsphdnvn@gmail.com>
|
❌ 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? |
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.