Throw if two inner_hits have the same name #37645

dvehar · 2019-01-21T07:55:46Z

elasticmachine · 2019-01-21T08:14:45Z

Pinging @elastic/es-search

jimczi

Thanks @dvehar , the change looks great.
I left some comments regarding the unit tests, we also need an integration test that check that setting two inner hits with the same name throws an exception during the execution of the query. Can you add one in NestedIT for instance ?

jimczi · 2019-01-21T08:22:35Z

modules/parent-join/src/test/java/org/elasticsearch/join/query/HasChildQueryBuilderTests.java

+
+        try {
+            queryBuilder.extractInnerHitBuilders(Collections.singletonMap("some_name", null));
+            fail("Expected an IllegalArgumentException to be thrown");


Can you use IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> queryBuilder.extractInnerHitBuilders(Collections.singletonMap("some_name", null)); instead ?

Can you change the test to use the static function InnerHitContextBuilder#extractInnerHits to retrieve the builders ? It's the function we use to build the inner hits map.

jimczi · 2019-01-21T08:22:52Z

modules/parent-join/src/test/java/org/elasticsearch/join/query/HasParentQueryBuilderTests.java

+
+        try {
+            queryBuilder.extractInnerHitBuilders(Collections.singletonMap("some_name", null));
+            fail("Expected an IllegalArgumentException to be thrown");


jimczi · 2019-01-21T08:23:02Z

server/src/test/java/org/elasticsearch/index/query/NestedQueryBuilderTests.java

+
+        try {
+            queryBuilder.extractInnerHitBuilders(Collections.singletonMap("some_name", null));
+            fail("Expected an IllegalArgumentException to be thrown");


and here ;)

dvehar · 2019-01-22T05:10:58Z

I'll work on the integration test soon

dvehar · 2019-01-24T03:14:35Z

@jimczi Could you provide some guidance as to what my test case should look like?

public void testExtractInnerHitBuildersWithDuplicate() throws Exception {
        assertAcked(
            prepareCreate("idxduplicatehitnames")
                .setSettings(Settings.builder().put(SETTING_NUMBER_OF_SHARDS, 1).put(SETTING_NUMBER_OF_REPLICAS, 0))
                .addMapping("product", "categories", "type=keyword", "name", "type=text", "property", "type=nested")
        );
        ensureGreen("idxduplicatehitnames");

        client().prepareIndex("idxduplicatehitnames", "product", "1").setSource(jsonBuilder().startObject()
            .field("name", "product1")
            .array("categories", "1", "2", "3", "4")
            .startArray("property")
            .startObject().field("id", 1).endObject()
            .startObject().field("id", 2).endObject()
            .startObject().field("id", 3).endObject()
            .endArray()
            .endObject()).get();
        client().prepareIndex("idxduplicatehitnames", "product", "2").setSource(jsonBuilder().startObject()
            .field("name", "product2")
            .array("categories", "1", "2")
            .startArray("property")
            .startObject().field("id", 1).endObject()
            .startObject().field("id", 5).endObject()
            .startObject().field("id", 4).endObject()
            .endArray()
            .endObject()).get();
        refresh();

//        SearchResponse response = client().prepareSearch("idxduplicatehitnames").setTypes("product")
//            .addAggregation(terms("category").field("categories").subAggregation(
//                nested("property", "property").subAggregation(
//                    terms("property_id").field("property.id")
//                )
//                .innerHit(new InnerHitBuilder())
//            ))
//            .innerHit(new InnerHitBuilder())
//            .get();

//        SearchResponse response = (SearchResponse)client().prepareSearch(new String[]{"test"})
//            .setQuery(boolQuery()
//                .must(matchQuery("key", "value"))
//                .should(
//                    nestedQuery("nested_type", matchAllQuery(), ScoreMode.None)
//                        .innerHit(new InnerHitBuilder()), ScoreMode.None)
//                .innerHit(new InnerHitBuilder()))
//            .get();

//
//        SearchResponse response = client().prepareSearch("idxduplicatehitnames").addAggregation(nested("to_method", "methods")
//            .subAggregation(filter("num_string_params1",
//                nestedQuery("methods.parameters", termQuery("methods.parameters.type", "String"), ScoreMode.None).innerHit(new InnerHitBuilder("n1"))))
//                .subAggregation(filter("num_string_params2",
//                    nestedQuery("methods.parameters", termQuery("methods.parameters.type", "String"), ScoreMode.None).innerHit(new InnerHitBuilder("n2"))))
//                .subAggregation(filter("num_string_params3",
//                    nestedQuery("methods.parameters", termQuery("methods.parameters.type", "String"), ScoreMode.None).innerHit(new InnerHitBuilder("n1"))))
//        ).get();

//        SearchResponse response = client()
//            .prepareSearch("idxduplicatehitnames")
//            .setTypes("product")
//            .addAggregation(
//                terms("category")
//                    .field("categories")
//                    .subAggregation(filter("n1", nestedQuery("property", termQuery("property.id", 1D), ScoreMode.None).innerHit(new InnerHitBuilder("ih1"))))
//                    .subAggregation(filter("n2", nestedQuery("property", termQuery("property.id", 2D), ScoreMode.None).innerHit(new InnerHitBuilder("ih2"))))
//                    .subAggregation(filter("n3", nestedQuery("property", termQuery("property.id", 5D), ScoreMode.None).innerHit(new InnerHitBuilder("ih1"))))
//            )
//            .get();


        SearchResponse response = client()
            .prepareSearch("idxduplicatehitnames")
            .setTypes("product")
            .addAggregation(
                terms("category")
                    .field("categories")
                    .subAggregation(filter("n1", nestedQuery("property", termQuery("property.id", 1D), ScoreMode.None).innerHit(new InnerHitBuilder("ih1"))))
                    .subAggregation(filter("n2", nestedQuery("property", termQuery("property.id", 2D), ScoreMode.None).innerHit(new InnerHitBuilder("ih2"))))
                    .subAggregation(filter("n3", nestedQuery("property", termQuery("property.id", 5D), ScoreMode.None).innerHit(new InnerHitBuilder("ih1")).innerHit(new InnerHitBuilder("ih1"))))
            )
            .addAggregation(
                terms("category2")
                    .field("categories")
                    .subAggregation(filter("n4", nestedQuery("property", termQuery("property.id", 1D), ScoreMode.None).innerHit(new InnerHitBuilder("ih1"))))
                    .subAggregation(filter("n5", nestedQuery("property", termQuery("property.id", 2D), ScoreMode.None).innerHit(new InnerHitBuilder("ih2"))))
                    .subAggregation(filter("n6", nestedQuery("property", termQuery("property.id", 5D), ScoreMode.None).innerHit(new InnerHitBuilder("ih1")).innerHit(new InnerHitBuilder("ih1"))))
            )
            .get();

        assertNoFailures(response);
//        assertHitCount(response, 2);
    }

I've tried a few things but can't seem to get NestedQueryBuilder::extractInnerHitBuilders to be called. I'll keep working on it meanwhile.

jimczi · 2019-01-24T10:25:50Z

I've tried a few things but can't seem to get NestedQueryBuilder::extractInnerHitBuilders to be called.

I tested locally and the following query failed with the check that you added in this pr:

SearchResponse response = client().prepareSearch("idxduplicatehitnames")
          .setQuery(boolQuery()
              .must(matchAllQuery())
              .should(nestedQuery("nested_type", matchAllQuery(), ScoreMode.None).innerHit(new InnerHitBuilder()))
              .should(nestedQuery("nested_type", matchAllQuery(), ScoreMode.None).innerHit(new InnerHitBuilder()))
          ).get();

You should not test aggregations, that's a different issue but we should not allow inner hits in the filter aggregations. They are not supported there so extractInnerHitBuilders is never called.

dvehar · 2019-01-25T07:27:02Z

@jimczi looks good?

jimczi

Thanks for updating @dvehar . I left more comments.

jimczi · 2019-01-25T08:41:04Z

modules/parent-join/src/main/java/org/elasticsearch/join/query/HasChildQueryBuilder.java

@@ -460,6 +460,10 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws I
    @Override
    protected void extractInnerHitBuilders(Map<String, InnerHitContextBuilder> innerHits) {
        if (innerHitBuilder != null) {
+            if (innerHits.containsKey(innerHitBuilder.getName())) {
+                throw new IllegalArgumentException("innerHits already contains an entry for key [" + innerHitBuilder.getName() + "]");


we should use the same format for the message than the option in the request, can you change innerHits to [inner_hits] ?

jimczi · 2019-01-25T08:41:11Z

modules/parent-join/src/main/java/org/elasticsearch/join/query/HasParentQueryBuilder.java

@@ -285,6 +285,10 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws I
    @Override
    protected void extractInnerHitBuilders(Map<String, InnerHitContextBuilder> innerHits) {
        if (innerHitBuilder != null) {
+            if (innerHits.containsKey(innerHitBuilder.getName())) {
+                throw new IllegalArgumentException("innerHits already contains an entry for key [" + innerHitBuilder.getName() + "]");


Same here, innerHits-> [inner_hits]

jimczi · 2019-01-25T08:41:30Z

server/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java

@@ -317,10 +318,14 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws
    @Override
    public void extractInnerHitBuilders(Map<String, InnerHitContextBuilder> innerHits) {
        if (innerHitBuilder != null) {
+            if (innerHits.containsKey(innerHitBuilder.getName())) {
+                throw new IllegalArgumentException("innerHits already contains an entry for key [" + innerHitBuilder.getName() + "]");


innerHits -> [inner_hits]

jimczi · 2019-01-25T08:46:05Z

modules/parent-join/src/main/java/org/elasticsearch/join/query/HasChildQueryBuilder.java

@@ -460,6 +460,10 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws I
    @Override
    protected void extractInnerHitBuilders(Map<String, InnerHitContextBuilder> innerHits) {
        if (innerHitBuilder != null) {
+            if (innerHits.containsKey(innerHitBuilder.getName())) {


If innerHitBuilder.getName() is null we use the type as the name (see below). Can you build the name first and check in the inner hits map with the inferred one ?:

String name = innerHitBuilder.getName() != null ? innerHitBuilder.getName() : type; if (innerHits.containsKey(name)) { ...

jimczi · 2019-01-25T08:47:03Z

modules/parent-join/src/main/java/org/elasticsearch/join/query/HasParentQueryBuilder.java

@@ -285,6 +285,10 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryShardContext) throws I
    @Override
    protected void extractInnerHitBuilders(Map<String, InnerHitContextBuilder> innerHits) {
        if (innerHitBuilder != null) {
+            if (innerHits.containsKey(innerHitBuilder.getName())) {


See above, we should check with the inferred name as well if innerHitBuilder.getName is null.

jimczi · 2019-01-25T08:47:43Z

server/src/main/java/org/elasticsearch/index/query/NestedQueryBuilder.java

@@ -317,10 +318,14 @@ protected QueryBuilder doRewrite(QueryRewriteContext queryRewriteContext) throws
    @Override
    public void extractInnerHitBuilders(Map<String, InnerHitContextBuilder> innerHits) {
        if (innerHitBuilder != null) {
+            if (innerHits.containsKey(innerHitBuilder.getName())) {


here the path is used if the name is null, can you infer the name first and then check inside the map ?

jimczi · 2019-01-25T08:49:26Z

server/src/test/java/org/elasticsearch/search/aggregations/bucket/NestedIT.java

+                .should(nestedQuery("property", termQuery("property.id", 1D), ScoreMode.None).innerHit(new InnerHitBuilder()))
+                .should(nestedQuery("property", termQuery("property.id", 1D), ScoreMode.None).innerHit(new InnerHitBuilder()))
+            ).get();
+        assertNoFailures(response);


This test should fail, we use the path of the nested field as the name when it is not provided so we should also detect the duplication here.

Ah ok. When you posted the example here I was thinking it should not fail. I'll fix this up.

ah ok, sorry it was not clear

no worries, I think it was just my limited understanding of things.

Fixes elastic#37584

jimczi · 2019-01-28T09:14:29Z

@elasticmachine test this please

jimczi · 2019-01-28T20:31:02Z

@elasticmachine test this please

jimczi

Thanks @dvehar , the changes look good to me. The tests are failing on unrelated issues so can you merge master in your branch (a simple merge) to make sure that we run the tests with the latest fixes ?

dvehar · 2019-01-28T21:23:39Z

@jimczi done

jimczi · 2019-01-28T21:43:58Z

@elasticmachine test this please

dvehar · 2019-01-28T23:26:52Z

@jimczi
Some builds are failing on org.elasticsearch.upgrades.FullClusterRestartIT.testRecovery(FullClusterRestartIT.java:842) because of java.lang.AssertionError: expected version to be one of [8.0.0,7.6.0] but was p 0 testrecovery 7.7.0. I can look into it more later. Let me know if you are aware of the issue

javanna · 2019-01-29T14:59:28Z

hi @dvehar that last test that was failing is now addressed. could you merge master in so we can re-run tests and hopefully have a green build? ;)

dvehar · 2019-01-29T16:30:49Z

@javanna done

jimczi · 2019-01-30T07:48:32Z

@elasticmachine test this please

javanna · 2019-01-30T10:13:12Z

retest this please

jimczi · 2019-01-31T20:52:28Z

retest this please

jimczi · 2019-02-01T08:44:57Z

retest this please

jimczi

Thanks @dvehar , CI is finally green. Thanks for iterating and sorry for the unrelated build failures ;).

This change throws an error if two inner_hits have the same name Closes elastic#37584

* master: Replace awaitBusy with assertBusy in atLeastDocsIndexed (elastic#38190) Adjust SearchRequest version checks (elastic#38181) AwaitsFix testClientSucceedsWithVerificationDisabled (elastic#38213) Zen2ify RareClusterStateIT (elastic#38184) ML: Fix error race condition on stop _all datafeeds and close _all jobs (elastic#38113) AwaitsFix PUT mapping with _doc on an index that has types (elastic#38204) Allow built-in monitoring_user role to call GET _xpack API (elastic#38060) Update geo_shape docs to include unsupported features (elastic#38138) [ML] Remove "8" prefixes from file structure finder timestamp formats (elastic#38016) Disable bwc tests while backporting elastic#38104 (elastic#38182) Enable TLSv1.3 by default for JDKs with support (elastic#38103) Fix _host based require filters (elastic#38173) RestoreService should update primary terms when restoring shards of existing indices (elastic#38177) Throw if two inner_hits have the same name (elastic#37645)

dvehar · 2019-02-01T18:35:07Z

@jimczi Thanks for helping me get it through!

This change throws an error if two inner_hits have the same name Closes #37584

…round-sync-6.x * elastic/6.x: Fix testRestoreIncreasesPrimaryTerms on 6.x (elastic#38314) SQL: Remove exceptions from Analyzer (elastic#38260) (elastic#38287) SQL: Move metrics tracking inside PlanExecutor (elastic#38259) (elastic#38288) Backport of elastic#38311: Move TokenService to seqno powered cas Handle scheduler exceptions (elastic#38183) Mute MlMigrationFullClusterRestartIT#testMigration (elastic#38316) 6.x Backport of elastic#38278: Move ML Optimistic Concurrency Control to Seq No Cleanup construction of interceptors (elastic#38296) Throw if two inner_hits have the same name (elastic#37645) (elastic#38194) AsyncTwoPhaseIndexerTests race condition fixed elastic#38195 Backport#37830 Enable SSL in reindex with security QA tests (elastic#38293) Ensure ILM policies run safely on leader indices (elastic#38140) Introduce ssl settings to reindex from remote (elastic#38292) Fix ordering problem in add or renew lease test (elastic#38281) Mute ReplicationTrackerRetentionLeaseTests#testAddOrRenewRetentionLease (elastic#38276) Fix NPE in Logfile Audit Filter (elastic#38120) (elastic#38271) Enable trace log in FollowerFailOverIT (elastic#38148) SQL: Generate relevant error message when grouping functions are not used in GROUP BY (elastic#38017)

jimczi added >bug :Search/Search Search-related issues that do not fall into other categories v7.0.0 v6.7.0 labels Jan 21, 2019

jimczi requested changes Jan 21, 2019

View reviewed changes

dvehar force-pushed the fix-37584 branch 5 times, most recently from aebb18b to b205f9d Compare January 25, 2019 07:26

jimczi requested changes Jan 25, 2019

View reviewed changes

dvehar added 2 commits January 25, 2019 09:25

throw if two inner_hits have the same name

9d1184f

Fixes elastic#37584

use inferred name; update error msg; update tests

823bb8d

dvehar force-pushed the fix-37584 branch from b205f9d to 823bb8d Compare January 25, 2019 17:36

revert whitespace change

c7d4623

Fix checkstyle issues

6760d76

jimczi reviewed Jan 28, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix-37584

114d927

Merge remote-tracking branch 'upstream/master' into fix-37584

5eafd01

Merge branch 'master' into pr/37645

ae0b4bd

Merge branch 'master' into pr/37645

3586cd4

jimczi approved these changes Feb 1, 2019

View reviewed changes

jimczi merged commit c1c4aba into elastic:master Feb 1, 2019

jimczi pushed a commit to jimczi/elasticsearch that referenced this pull request Feb 1, 2019

Throw if two inner_hits have the same name (elastic#37645)

5b792de

This change throws an error if two inner_hits have the same name Closes elastic#37584

jimczi added the backport pending label Feb 1, 2019

jimczi mentioned this pull request Feb 1, 2019

Throw if two inner_hits have the same name #38194

Merged

dvehar deleted the fix-37584 branch February 1, 2019 18:35

jimczi added a commit that referenced this pull request Feb 4, 2019

Throw if two inner_hits have the same name (#37645) (#38194)

ebfafcf

This change throws an error if two inner_hits have the same name Closes #37584

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

jimczi removed the backport pending label Feb 7, 2019

jimczi mentioned this pull request Mar 28, 2019

In hierarchical documents, under certain scenarios, snippets for descendant documents don't turn up #40572

Closed

jimczi mentioned this pull request Apr 18, 2019

Inner hits should fail when the same name is used multiple times in the same query #21055

Closed

jimczi mentioned this pull request Jan 23, 2024

elasticsearch hung up / closing connection during collapse search query with size parameter #104647

Closed

Throw if two inner_hits have the same name #37645

Throw if two inner_hits have the same name #37645

Conversation

dvehar commented Jan 21, 2019

elasticmachine commented Jan 21, 2019

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvehar commented Jan 22, 2019

dvehar commented Jan 24, 2019

jimczi commented Jan 24, 2019 • edited Loading

dvehar commented Jan 25, 2019

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimczi commented Jan 28, 2019

jimczi commented Jan 28, 2019

jimczi left a comment

Choose a reason for hiding this comment

dvehar commented Jan 28, 2019

jimczi commented Jan 28, 2019

dvehar commented Jan 28, 2019

javanna commented Jan 29, 2019

dvehar commented Jan 29, 2019

jimczi commented Jan 30, 2019

javanna commented Jan 30, 2019

jimczi commented Jan 31, 2019

jimczi commented Feb 1, 2019

jimczi left a comment

Choose a reason for hiding this comment

dvehar commented Feb 1, 2019

jimczi commented Jan 24, 2019 •

edited

Loading