Make 0 as invalid value for min_children in has_child query
#41347
Conversation
There was a problem hiding this comment.
Hi @codebird, thanks for the PR. I left a minor comment around the error message. Also at least the unit test HasChildQueryBuilderTests needs adapting to the new illegal values and message. Maybe other tests are failing. You can take a look at the TESTING.txt file if you are unsure of how to run the tests. Since the change is in the "server" subproject, I think running "./gradlew -p server check" from the root folder should catch most test failures. Otherwise we'll catch them when running the final CI tests. Let me know if you have any questions.
modules/parent-join/src/main/java/org/elasticsearch/join/query/HasChildQueryBuilder.java
Outdated
Show resolved
Hide resolved
cbuescher
added
:Search/Search
|
Pinging @elastic/es-search |
cbuescher
changed the title
min_children in has_child query
|
This relates to #32949. |
|
Another quick hint: apart from changing the min/max value cases in HasChildQueryBuilderTests#testIllegalValues, the #doCreateTestQueryBuilder method in the same test class will need some changes so we don't draw 0 values for min/max when randomly setting up the query builder. |
|
@codebird the change looks good from what I see, unfortunately the diff is a bit large now with the unrelated changes merged in from master. I'm not sure this is a Github quirk, but could you try squashing your branch down to one commit and force-push that to your PR branch? That would make it easier to read I think. I will kick of the larger CI test suite after that. Thanks. |
|
done @cbuescher Thanks for the help |
|
Something is weird. Your branch still shows changes in unrelated classes (e.g. TestingConventionsTasks.java), but when I look at the current state of that class on master it looks like that version is already similar with the one on your PR branch. Maybe a Github glitch. I will need to look into this more deeply before merging, but will run CI for now. |
|
@cbuescher that's because I did shouldn't have done it on the branch... |
…n case min_children is not provided it will be set to 1 by default.
|
I updated DEFAULT_MIN_CHILDREN to be 1 not 0, for the case where min_children is not provided at all, that's what was causing CI to fail. @cbuescher |
|
@codebird I hope you don't mind but I merged in master once more to fix a merge conflict. Now the diff also looks okay (only two files changed). Will run the full tests again. |
|
@elasticmachine test this please |
|
@cbuescher I think you need to re-initiate the tests. if I mention elasticmachine and say test this please will it run the tests? |
|
Unfortunately not. You need to be member of the "Elastic" organization to run the CI bot so our testing infrastructure cannot be DoS'ed. The current ci/1 failure seems unrelated to this change, I will just run that part again. |
|
cute this elasticmachine :) Yeah I know it is not related to us, that's why I said you need to re-initiate |
|
@elasticmachine run elasticsearch-ci/1 |
|
The last ci/1 run died a while ago with "java.lang.OutOfMemoryError: Java heap space" https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-1/12487/console |
|
Failed again with errors not to do with this |
|
Unfortunately. Just for reference, the last ones were timeouts: https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-1/12510/ |
|
hmmm weird, it failed again
Just for my info does this always happen like this? Or something causing it now, systems are busy or something? |
|
@codebird normally things pass more smoothly. We had some recent changes in the build system that might make this one a bit more bumpy. The latest failures are unrelated as well and look transient (some googleapis 500 Internal Server Errors in GCE plugin tests). This is unfortunate, but we want to have at least one clear run of the entire suite before merging anything, even if its only a few lines changed like this one. |
|
@elasticmachine run elasticsearch-ci/1 |
|
@codebird ^ latest one is gce failure again, feels like a general problem in the testing infra at the moment (not just this PR). I'm going to wait on internal feedback before continuing re-runs here. |
|
Merged master, maybe that helps. |
|
Wooooo it worked |
|
Thanks for the help @cbuescher. And sorry for the headache :P |
|
No problem, and thanks for picking this up, much appreciated. I would like to backport this to 7.1 but I see problems with this since it would mean introducing a slightly different behaviour for the |
|
Maybe we should start with a warning on 7.1 just to prepare for this to go live some day. I don't know what are your rules for these warnings... |
|
That would be a nice follow up indeed. Are you interested in doing that in a small PR against the 7.x branch? Look at the way DeprecationLogger is used e.g. in TermsQueryBuilder. Adding something to the "minMaxChildren" method and log a short notice for 0 values (that this will be rejected in 8.0 but if using 1 this should behave the same). In that case we'd also need to adapt tests in a similar way like in this PR because triggering the warning will cause tests to fail. In additon we'd need a test that checks that the 0 value triggers the warning. See e.g. TermsQueryBuilderTests#testTypeField how this can be done using the "assertWarnings" method. |
|
Yes interested since this is not urgent and a bit busy at work (and obviously I am still slowly learning the code) . Let's do it |
|
Great, I opened #41548 just so we don't forget following up on it. Feel free to grab it and let me know if there are any questions. |
…s-in-all-the-places * elastic/master: (70 commits) Remove experimental label froms script_score query (elastic#41572) [Ml-Dataframe] Update URLs in Data frame client java doc (elastic#41539) Reenable bwc Tests in master (elastic#41540) Fix search_as_you_type's sub-fields to pick their names from the full path of the root field (elastic#41541) Remove search analyzers from DocumentFieldMappers (elastic#41484) Update community client and integration docs (elastic#41513) Remove Version.V_6_x_x constants use in security (elastic#41185) Mute testDriverConfigurationWithSSLInURL Remove dedicated SSL network write buffer (elastic#41283) Disable max score optimization for queries with unbounded max scores (elastic#41361) [DOCS] Explicitly set section IDs for Asciidoctor migration (elastic#41547) [ML] add multi node integ tests for data frames (elastic#41508) [Docs] Fix common word repetitions (elastic#39703) [DOCS] Note TESTRESPONSE can't be used immediately after TESTSETUP (elastic#41542) Update configuring-ldap-realm.asciidoc (elastic#40427) Fixed very small typo in date (elastic#41398) Refactor GeoHashUtils (elastic#40869) Make 0 as invalid value for `min_children` in `has_child` query (elastic#41347) field_caps: adapt bwc version after backport (elastic#41427) Remove Exists Check from S3 Repository Deletes (elastic#40931) ...
…tic#41347) * squashing multiple commits * fixing elastic#32949 updated DEFAULT_MIN_CHILDREN to be 1, this way in case min_children is not provided it will be set to 1 by default. * Fix ChildQuerySearchIT
…tic#41347) * squashing multiple commits * fixing elastic#32949 updated DEFAULT_MIN_CHILDREN to be 1, this way in case min_children is not provided it will be set to 1 by default. * Fix ChildQuerySearchIT
Just updated the test in file
modules/parent-join/src/main/java/org/elasticsearch/join/query/HasChildQueryBuilder.java
from
< 0to<= 0and changed the error message.