Make _index optional for pinned query docs #97450

ioanatia · 2023-07-07T13:19:19Z

Currently pinned queries require either the ids or docs parameter.
docs allows pinning documents from specific indices.
However for docs the _index field is always required:

GET test/_search
{
  "query": {
    "pinned": {
      "organic": {
        "query_string": {
          "query": "something"
        }
      },
      "docs": [
        { "_id": "1" }
      ]
    }
  }
}

returns an error:

{
  "error": {
    "root_cause": [
      {
        "type": "parsing_exception",
        "reason": "[10:22] [pinned] failed to parse field [docs]",
        "line": 10,
        "col": 22
      }
    ],
    "type": "parsing_exception",
    "reason": "[10:22] [pinned] failed to parse field [docs]",
    "line": 10,
    "col": 22,
    "caused_by": {
      "type": "x_content_parse_exception",
      "reason": "[10:22] [pinned] failed to parse field [docs]",
      "caused_by": {
        "type": "illegal_argument_exception",
        "reason": "Required [_index]"
      }
    }
  },
  "status": 400
}

The proposal here is to make _index optional. I don't think we have a strong requirement for making _index required, when it was initially introduced in #74873, we mostly wanted the ability to pin docs from specific indices.

Making _index optional can give more flexibility to use a combination of pinned documents from specific indices or just document ids.
This change can also help with pinned query rules. Currently pinned query rules can accept either ids or docs.
If multiple pinned query rules match and they use a combination of ids and docs, we cannot build a pinned query and we would need to return an error. This is because a pinned query cannot accept both ids and docs.
By making _index optional we would no longer need to return an error when pinned query rules use a combination of ids and docs, because we can easily translate ids in docs.

The following pinned queries would be equivalent:

GET test/_search
{
  "query": {
    "pinned": {
      "organic": {
        "query_string": {
          "query": "something"
        }
      },
      "docs": [
        { "_id": "1" }
      ]
    }
  }
}

GET test/_search
{
  "query": {
    "pinned": {
      "organic": {
        "query_string": {
          "query": "something"
        }
      },
      "ids": [1]
    }
  }
}

The scores should be consistent when using a combination of _docs that might use _index or not - see example

Example

PUT test-1/_doc/1
{
 "title": "doc 1"
}

PUT test-1/_doc/2
{
 "title": "doc 2"
}

PUT test-2/_doc/1
{
 "title": "doc 1"
}

PUT test-2/_doc/3
{
 "title": "lalala"
}


POST test-1,test-2/_search
{
 "query": {
   "pinned": {
     "organic": {
       "query_string": {
         "query": "lalala"
       }
     },
     "docs": [
       { "_id": "2", "_index": "test-1" },
       { "_id": "1" }
     ]
   }
 }
}

response:

{
 "took": 1,
 "timed_out": false,
 "_shards": {
   "total": 2,
   "successful": 2,
   "skipped": 0,
   "failed": 0
 },
 "hits": {
   "total": {
     "value": 4,
     "relation": "eq"
   },
   "max_score": 1.7014124e+38,
   "hits": [
     {
       "_index": "test-1",
       "_id": "2",
       "_score": 1.7014124e+38,
       "_source": {
         "title": "doc 2"
       }
     },
     {
       "_index": "test-1",
       "_id": "1",
       "_score": 1.7014122e+38, // same score as doc with id 1 from test-2
       "_source": {
         "title": "doc 1"
       }
     },
     {
       "_index": "test-2",
       "_id": "1",
       "_score": 1.7014122e+38, // same score as doc with id 1 from test-1
       "_source": {
         "title": "doc 1"
       }
     },
     {
       "_index": "test-2",
       "_id": "3",
       "_score": 0.8025915, // organic result
       "_source": {
         "title": "lalala"
       }
     }
   ]
 }
}

For query rules, if we have two query rules that both match and use a combination of ids and pinned:

PUT _query_rules/test-ruleset
{
  "ruleset_id": "test-ruleset",
  "rules": [
    {
      "rule_id": "1",
      "type": "pinned",
      "criteria": [
        {
          "type": "exact",
          "metadata": "query_string",
          "value": "country"
        }
      ],
      "actions": {
        "docs": [
          { "_index": "singers", "_id": "1" }
        ]
      }
    },
    {
      "rule_id": "2",
      "type": "pinned",
      "criteria": [
        {
          "type": "exact",
          "metadata": "query_string",
          "value": "country"
        }
      ],
      "actions": {
        "ids": [
          2
        ]
      }
    }
  ]
}

and the following query:

POST singers/_search
{
    "query": {
        "rule_query": {
            "organic": {
                "query_string": {
                  "default_field": "name",
                  "query": "country"
                }
            },
            "match_criteria": {
                "query_string": "country"
            },
            "ruleset_id": "test-ruleset"
        }
    }
}

then this would get translated into the following pinned query:

POST singers/_search
{
    "query": {
        "pinned": {
            "organic": {
                "query_string": {
                  "default_field": "name",
                  "query": "country"
                }
            },
            "docs": [
               { "_index": "singers", "_id": "1" },
               {"_id": 2 }
            ]
        }
    }
}

I think we can also simplify the pinned query rule so that it only receives docs:

PUT _query_rules/test-ruleset
{
  "ruleset_id": "test-ruleset",
  "rules": [
    {
      "rule_id": "1",
      "type": "pinned",
      "criteria": [
        {
          "type": "exact",
          "metadata": "query_string",
          "value": "country"
        }
      ],
      "actions": {
        "docs": [
          { "_id": "1" },
          { "_id": "2", "_index": "singers" }
        ]
      }
    }
  ]
}

kderusso · 2023-07-07T14:03:24Z

I think this change makes sense, but we should probably update the pinned query docs as well if we make this change:

Denote docs as the preferred way to query pinned documents
Specify that _index is optional
Note that colliding _ids in both indices will both be pinned if _index is not specified

ioanatia · 2023-07-24T13:33:22Z

There is one side effect that I want to call out here, it is possible to pin the same documents multiple times with the pinned query - however the results will not contain duplicate pinned documents. This example should explain it better:

PUT test1/_doc/1
{
  "title": "document 1"
}

PUT test2/_doc/1
{
  "title": "document 1"
}

GET test1,test2/_search
{
  "query": {
    "pinned": {
      "docs": [
        {
          "_index": "test1",
          "_id": "1"
        },
        {
          "_id": "1" // this pins document with id `id` from test1 again
        }
      ],
      "organic": {
        "match": {
          "description": "document"
        }
      }
    }
  }
}

results:

 "hits": {
    "total": {
      "value": 2,
      "relation": "eq"
    },
    "max_score": 1.7014124e+38,
    "hits": [
      {
        "_index": "test1",
        "_id": "1",
        "_score": 1.7014124e+38, // notice that the first hit has a higher score and it's the right pinned result
        "_source": {
          "title": "document 1"
        }
      },
      {
        "_index": "test2",
        "_id": "1",
        "_score": 1.7014122e+38,
        "_source": {
          "title": "document 1"
        }
      }
    ]
  }

I think this should be okay, because a similar behaviour can be achieved on main with the use of aliases:

POST _aliases
{
  "actions": [
    {
      "add": {
        "indices": ["test1", "test2"],
        "alias": "my-alias"
      }
    }
  ]
}

GET my-alias/_search
{
  "query": {
    "pinned": {
      "docs": [
        {
          "_index": "test1",
          "_id": "1"
        },
        {
          "_id": "1",
          "_index": "my-alias" // this also pins the document from test1
        } 
      ],
      "organic": {
        "match": {
          "description": "document"
        }
      }
    }
  }
}

results:

 "hits": [
      {
        "_index": "test1",
        "_id": "1",
        "_score": 1.7014124e+38, // again, the first pinned document has the highest score
        "_source": {
          "title": "document 1"
        }
      },
      {
        "_index": "test2",
        "_id": "1",
        "_score": 1.7014122e+38,
        "_source": {
          "title": "document 1"
        }
      }
    ]

elasticsearchmachine · 2023-07-24T14:16:22Z

Pinging @elastic/es-search (Team:Search)

elasticsearchmachine · 2023-07-24T14:17:24Z

Hi @ioanatia, I've created a changelog YAML for you.

saikatsarkar056 · 2023-08-17T16:59:47Z

I've noticed that the actual GitHub issue isn't mentioned in this PR. Is this because it's a public repository? 🤔

saikatsarkar056 · 2023-08-17T17:03:31Z

What does organic match mean? I am trying to understand the meaning by reading this doc on pinned query. Is there any other related documentation?

kderusso · 2023-08-17T17:11:14Z

@saikatsarkar056

I've noticed that the actual GitHub issue isn't mentioned in this PR. Is this because it's a public repository? 🤔

Yes.

What does organic match mean? I am trying to understand the meaning by reading this doc on pinned query. Is there any other related documentation?

The organic query is the query that is run, the results are underneath all pinned documents.

benwtrent · 2023-08-28T11:35:45Z

...ness-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/PinnedQueryBuilder.java

@@ -105,13 +102,13 @@ private Item(String id) {
         * Read from a stream.
         */
        public Item(StreamInput in) throws IOException {
-            index = in.readString();
+            index = in.readOptionalString();


BWC serialization must be handled on input and output.

benwtrent · 2023-08-28T11:36:23Z

...ugin/src/yamlRestTest/resources/rest-api-spec/test/search-business-rules/10_pinned_query.yml

+  - skip:
+      version: " - 8.9.99"
+      reason: "'_index' was made optional in 8.10"


This needs bumped to 8.10.99 if this is still going to be merged.

benwtrent

PinnedQueryBuilderTests should be updated to account for the index being randomly null or not when generating a query.

benwtrent · 2023-08-29T12:20:43Z

...ness-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/PinnedQueryBuilder.java

+            if (out.getTransportVersion().onOrAfter(OPTIONAL_INDEX_IN_DOCS_VERSION)) {
+                out.writeOptionalString(index);
+            } else {
+                out.writeString(index);


This will throw an NPE. It is possible that the coordinator is upgraded to the latest version of Elasticsearch but the relevant data nodes are not. Consequently, if the user does not provide an _index, the coordinator will accept the call, attempt to serialize, and an NPE will throw.

We should instead be nice here and throw a better error message.

Good catch - I've included an IllegalArgumentException that hopefully makes sense 👍

benwtrent · 2023-08-29T12:21:25Z

...ness-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/PinnedQueryBuilder.java

@@ -129,7 +126,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
        );

        static {
-            PARSER.declareString(constructorArg(), INDEX_FIELD);
+            PARSER.declareStringOrNull(optionalConstructorArg(), INDEX_FIELD);


declareStringOrNull

This means we allow the value to be string or explicitly null. I think we instead allow it to be string or not provided at all. Consequently, it should just be PARSER.declareString(optionalConstructorArg(), INDEX_FIELD);

benwtrent · 2023-08-29T12:22:07Z

...ness-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/PinnedQueryBuilder.java

+                out.writeOptionalString(index);
+            } else {
+                out.writeString(index);
+            }


Below in the toXContent method, we should not write the INDEX_FIELD if this.index == null.

Done, thanks!

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersion.java

carlosdelest · 2023-09-04T14:19:05Z

PinnedQueryBuilderTests should be updated to account for the index being randomly null or not when generating a query.

Thanks for checking this - I've added that to the tests 👍

carlosdelest · 2023-09-05T09:44:49Z

@elasticmachine update branch

elasticmachine · 2023-09-05T09:44:51Z

merge conflict between base and head

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersion.java

kderusso

This looks good to me, thanks for taking it over the line!

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersion.java

Currently pinned queries require either the `ids` or `docs` parameter. `docs` allows pinning documents from specific indices. However for `docs` the `_index` field is always required: ``` GET test/_search { "query": { "pinned": { "organic": { "query_string": { "query": "something" } }, "docs": [ { "_id": "1" } ] } } } ``` returns an error: ``` { "error": { "root_cause": [ { "type": "parsing_exception", "reason": "[10:22] [pinned] failed to parse field [docs]", "line": 10, "col": 22 } ], "type": "parsing_exception", "reason": "[10:22] [pinned] failed to parse field [docs]", "line": 10, "col": 22, "caused_by": { "type": "x_content_parse_exception", "reason": "[10:22] [pinned] failed to parse field [docs]", "caused_by": { "type": "illegal_argument_exception", "reason": "Required [_index]" } } }, "status": 400 } ``` The proposal here is to make `_index` optional. I don't think we have a strong requirement for making `_index` required, when it was initially introduced in elastic#74873, we mostly wanted the ability to pin docs from specific indices. Making `_index` optional can give more flexibility to use a combination of pinned documents from specific indices or just document ids. This change can also help with pinned query rules. Currently pinned query rules can accept either `ids` or `docs`. If multiple pinned query rules match and they use a combination of `ids` and `docs`, we cannot build a pinned query and we would need to return an error. This is because a pinned query cannot accept both `ids` and `docs`. By making `_index` optional we would no longer need to return an error when pinned query rules use a combination of `ids` and `docs`, because we can easily translate `ids` in `docs`. The following pinned queries would be equivalent: ``` GET test/_search { "query": { "pinned": { "organic": { "query_string": { "query": "something" } }, "docs": [ { "_id": "1" } ] } } } GET test/_search { "query": { "pinned": { "organic": { "query_string": { "query": "something" } }, "ids": [1] } } } ``` The scores should be consistent when using a combination of _docs that might use _index or not - see example <details> <summary>Example </summary> ``` PUT test-1/_doc/1 { "title": "doc 1" } PUT test-1/_doc/2 { "title": "doc 2" } PUT test-2/_doc/1 { "title": "doc 1" } PUT test-2/_doc/3 { "title": "lalala" } POST test-1,test-2/_search { "query": { "pinned": { "organic": { "query_string": { "query": "lalala" } }, "docs": [ { "_id": "2", "_index": "test-1" }, { "_id": "1" } ] } } } ``` response: ``` { "took": 1, "timed_out": false, "_shards": { "total": 2, "successful": 2, "skipped": 0, "failed": 0 }, "hits": { "total": { "value": 4, "relation": "eq" }, "max_score": 1.7014124e+38, "hits": [ { "_index": "test-1", "_id": "2", "_score": 1.7014124e+38, "_source": { "title": "doc 2" } }, { "_index": "test-1", "_id": "1", "_score": 1.7014122e+38, // same score as doc with id 1 from test-2 "_source": { "title": "doc 1" } }, { "_index": "test-2", "_id": "1", "_score": 1.7014122e+38, // same score as doc with id 1 from test-1 "_source": { "title": "doc 1" } }, { "_index": "test-2", "_id": "3", "_score": 0.8025915, // organic result "_source": { "title": "lalala" } } ] } } ``` </details> For query rules, if we have two query rules that both match and use a combination of `ids` and `pinned`: ``` PUT _query_rules/test-ruleset { "ruleset_id": "test-ruleset", "rules": [ { "rule_id": "1", "type": "pinned", "criteria": [ { "type": "exact", "metadata": "query_string", "value": "country" } ], "actions": { "docs": [ { "_index": "singers", "_id": "1" } ] } }, { "rule_id": "2", "type": "pinned", "criteria": [ { "type": "exact", "metadata": "query_string", "value": "country" } ], "actions": { "ids": [ 2 ] } } ] } ``` and the following query: ``` POST singers/_search { "query": { "rule_query": { "organic": { "query_string": { "default_field": "name", "query": "country" } }, "match_criteria": { "query_string": "country" }, "ruleset_id": "test-ruleset" } } } ``` then this would get translated into the following pinned query: ``` POST singers/_search { "query": { "pinned": { "organic": { "query_string": { "default_field": "name", "query": "country" } }, "docs": [ { "_index": "singers", "_id": "1" }, {"_id": 2 } ] } } } ``` I think we can also simplify the pinned query rule so that it only receives `docs`: ``` PUT _query_rules/test-ruleset { "ruleset_id": "test-ruleset", "rules": [ { "rule_id": "1", "type": "pinned", "criteria": [ { "type": "exact", "metadata": "query_string", "value": "country" } ], "actions": { "docs": [ { "_id": "1" }, { "_id": "2", "_index": "singers" } ] } } ] } ```

elasticsearchmachine added the v8.10.0 label Jul 7, 2023

Relax requirement for pinned query docs and make _index optional

376a86f

ioanatia force-pushed the ioana/pinned_query branch from 182cbd1 to 0a94cbb Compare July 24, 2023 11:10

ioanatia added 2 commits July 24, 2023 13:43

Add yaml test

3480959

Modify documentation

95267d1

ioanatia force-pushed the ioana/pinned_query branch from 0a94cbb to 95267d1 Compare July 24, 2023 11:58

Add test for duplicate pinned documents with aliases

39fe714

ioanatia marked this pull request as ready for review July 24, 2023 14:14

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Jul 24, 2023

ioanatia added the Team:Enterprise Search Meta label for Enterprise Search team label Jul 24, 2023

ioanatia requested review from jimczi and a team July 24, 2023 14:15

elasticsearchmachine removed the Team:Enterprise Search Meta label for Enterprise Search team label Jul 24, 2023

ioanatia added :Search/Search Search-related issues that do not fall into other categories Team:Enterprise Search Meta label for Enterprise Search team labels Jul 24, 2023

elasticsearchmachine added Team:Search Meta label for search team and removed needs:triage Requires assignment of a team area label Team:Enterprise Search Meta label for Enterprise Search team labels Jul 24, 2023

ioanatia added >enhancement needs:triage Requires assignment of a team area label Team:Enterprise Search Meta label for Enterprise Search team and removed Team:Search Meta label for search team labels Jul 24, 2023

elasticsearchmachine added Team:Search Meta label for search team and removed needs:triage Requires assignment of a team area label Team:Enterprise Search Meta label for Enterprise Search team labels Jul 24, 2023

Update docs/changelog/97450.yaml

f008901

Merge branch 'main' into ioana/pinned_query

5242e8e

benwtrent reviewed Aug 28, 2023

View reviewed changes

carlosdelest added 4 commits August 28, 2023 17:58

Merge branch 'main' into ioana/pinned_query

1a76f54

Add serialization for different transport versions

3315422

Fix spotless

d2b38a6

Add bad request test for not including doc _index in previous version

cdc6268

carlosdelest self-assigned this Aug 29, 2023

benwtrent reviewed Aug 29, 2023

View reviewed changes

carlosdelest added 2 commits September 4, 2023 16:15

Merge remote-tracking branch 'origin/main' into ioana/pinned_query

6f9c5cd

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersion.java

PR feedback - take into account index can be null

d852a8e

carlosdelest added 3 commits September 5, 2023 11:46

Merge remote-tracking branch 'origin/main' into ioana/pinned_query

9d68823

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersion.java

Update TransportAction

a40cf67

Bring back original test, without forcing indices

d4102c9

kderusso approved these changes Sep 5, 2023

View reviewed changes

carlosdelest requested a review from benwtrent September 5, 2023 14:28

benwtrent approved these changes Sep 6, 2023

View reviewed changes

carlosdelest added 2 commits September 6, 2023 20:48

Merge branch 'main' into ioana/pinned_query

881cfef

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersion.java

Update TransportVersion

49ed0c2

carlosdelest added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 6, 2023

carlosdelest added 2 commits September 7, 2023 09:45

Merge branch 'main' into ioana/pinned_query

852570a

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersion.java

Update TransportVersion. Again.

6d3eeed

elasticsearchmachine merged commit 7cd1987 into elastic:main Sep 7, 2023
12 checks passed

ioanatia deleted the ioana/pinned_query branch September 7, 2023 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make _index optional for pinned query docs #97450

Make _index optional for pinned query docs #97450

ioanatia commented Jul 7, 2023 •

edited

kderusso commented Jul 7, 2023

ioanatia commented Jul 24, 2023

elasticsearchmachine commented Jul 24, 2023

elasticsearchmachine commented Jul 24, 2023

saikatsarkar056 commented Aug 17, 2023

saikatsarkar056 commented Aug 17, 2023

kderusso commented Aug 17, 2023 •

edited

benwtrent Aug 28, 2023

benwtrent Aug 28, 2023

benwtrent left a comment

benwtrent Aug 29, 2023

carlosdelest Sep 4, 2023

benwtrent Aug 29, 2023

benwtrent Aug 29, 2023

carlosdelest Sep 4, 2023

carlosdelest commented Sep 4, 2023

carlosdelest commented Sep 5, 2023

elasticmachine commented Sep 5, 2023

kderusso left a comment

Make _index optional for pinned query docs #97450

Make _index optional for pinned query docs #97450

Conversation

ioanatia commented Jul 7, 2023 • edited

kderusso commented Jul 7, 2023

ioanatia commented Jul 24, 2023

elasticsearchmachine commented Jul 24, 2023

elasticsearchmachine commented Jul 24, 2023

saikatsarkar056 commented Aug 17, 2023

saikatsarkar056 commented Aug 17, 2023

kderusso commented Aug 17, 2023 • edited

benwtrent Aug 28, 2023

Choose a reason for hiding this comment

benwtrent Aug 28, 2023

Choose a reason for hiding this comment

benwtrent left a comment

Choose a reason for hiding this comment

benwtrent Aug 29, 2023

Choose a reason for hiding this comment

carlosdelest Sep 4, 2023

Choose a reason for hiding this comment

benwtrent Aug 29, 2023

Choose a reason for hiding this comment

benwtrent Aug 29, 2023

Choose a reason for hiding this comment

carlosdelest Sep 4, 2023

Choose a reason for hiding this comment

carlosdelest commented Sep 4, 2023

carlosdelest commented Sep 5, 2023

elasticmachine commented Sep 5, 2023

kderusso left a comment

Choose a reason for hiding this comment

ioanatia commented Jul 7, 2023 •

edited

kderusso commented Aug 17, 2023 •

edited