[Query rules] Add `exclude` query rule type #111420

kderusso · 2024-07-29T18:21:53Z

Adds a new type of query rule, exclude. Unlike the pinned rule, which pins specified documents to the top of result sets, the exclude query rule will identify documents that we never want returned in the result sets.

Here's an example of a ruleset with both a pinned and an exclude rule:

PUT /_query_rules/test-ruleset
{
  "rules": [
    {
      "rule_id": "1",
      "type": "pinned",
      "criteria": [
        {
          "type": "exact",
          "metadata": "query-string",
          "values": [
            "foo"
          ]
        }
      ],
      "actions": {
        "ids": [
          "1"
        ]
      }
    },
    {
      "rule_id": "2",
      "type": "exclude",
      "criteria": [
        {
          "type": "exact",
          "metadata": "query-string",
          "values": [
            "bar"
          ]
        }
      ],
      "actions": {
        "ids": [
          "2"
        ]
      }
    }
  ]
}

…ied docs

…uments from result sets

elasticsearchmachine · 2024-07-29T18:22:43Z

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

kderusso · 2024-07-29T18:26:47Z

...iness-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/SpecifiedDocument.java

Moved out of PinnedQueryBuilder without additional change.

elasticsearchmachine · 2024-07-29T20:24:27Z

Pinging @elastic/search-relevance (Team:Search - Relevance)

elasticsearchmachine · 2024-07-29T20:24:27Z

Pinging @elastic/ent-search-eng (Team:SearchOrg)

markjhoy · 2024-07-29T20:47:14Z

docs/reference/query-rules/apis/put-query-rule.asciidoc

@@ -102,9 +105,9 @@ If multiple matching rules pin more than 100 documents, only the first 100 docum
 [[put-query-rule-example]]
 ==== {api-examples-title}

-The following example creates a new query rule with the ID `my-rule1` in a query ruleset called `my-ruleset`.
+The following example creates a new query rule in a query ruleset called `my-ruleset`.


I'd still mention something regarding my-rule1 and where that comes from (I assume it's an arbitrary ID for the name of the rule within the my-ruleset ruleset?)

markjhoy · 2024-07-29T20:51:06Z

docs/reference/query-rules/apis/put-query-rule.asciidoc

 Only one of `ids` or `docs` may be specified, and at least one must be specified.
- `docs` (Optional, array of objects) The documents to pin.
+- `docs` (Optional, array of objects) The specified documents.


Is the max # of documents always hardcoded to 100? If so, I'd mention that here too

markjhoy · 2024-07-29T20:53:40Z

x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java

-                throw new ElasticsearchParseException("pinned hits cannot exceed " + MAX_NUM_PINNED_HITS);
+                throw new ElasticsearchParseException(type + " query rule actions cannot be empty");
+            } else if (((List<?>) action).size() > MAX_NUM_DOCS_IN_RULE) {
+                throw new ElasticsearchParseException(type + " documents cannot exceed " + MAX_NUM_DOCS_IN_RULE);


Might it be worth also validating to ensure the doc IDs are all unique in the list? Or, keep that onus on the user?

That's a good callout. My only concern is that technically that might be considered a breaking change, because existing rulesets with duplicate rules could now break. We do dedup the rules later on when they are applied. I'm inclined to leave it as is just to avoid breaking existing rules, but let me know if you'd like to chat about this more.

kderusso · 2024-07-30T18:50:50Z

@elasticmachine update branch

Mikep86

Nice work! Mostly small stuff, except I have a sneaking suspicion there's a bug with multiple exclude rules.

docs/reference/query-rules/apis/put-query-rule.asciidoc

docs/reference/query-rules/apis/put-query-ruleset.asciidoc

...lugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java

...iness-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/SpecifiedDocument.java

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

.../rest/src/yamlRestTest/resources/rest-api-spec/test/entsearch/rules/40_rule_query_search.yml

kderusso · 2024-08-01T15:47:06Z

@elasticmachine update branch

...iness-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/SpecifiedDocument.java

docs/reference/query-rules/apis/put-query-ruleset.asciidoc

x-pack/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/QueryRule.java

Mikep86 · 2024-08-01T18:42:35Z

...gin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/RuleQueryBuilder.java

+        List<SpecifiedDocument> pinnedDocs = pinnedDocsSupplier != null ? pinnedDocsSupplier.get() : null;
+        if (pinnedDocs != null && pinnedDocs.isEmpty() == false) {
+            PinnedQueryBuilder pinnedQueryBuilder = new PinnedQueryBuilder(organicQuery, pinnedDocs.toArray(new SpecifiedDocument[0]));


Ok, I think I figured out the issue:

As of the rule query rename (#108831), RuleQueryBuilder deserialization was changed to throw away pinnedIds and pinnedDocs values. If I'm reading the code correctly, I think BwC with 8.10.0 - 8.12.1 coordinator nodes was broken here, but it wasn't spotted at the time.

These changes further break BwC (although if you consider "broken" as a binary state, it was already broken) because pinnedDocsSupplier will always be null. The end effect is that any rule query from an 8.10.0 - 8.12.1 coordinator node will be handled as if none of the criteria matched (i.e. just run the organic query).

...gin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/RuleQueryBuilder.java

...lugin/ent-search/src/test/java/org/elasticsearch/xpack/application/rules/QueryRuleTests.java

Mikep86

A couple small follow-ups, plus maybe BwC is broken? Hope i'm wrong on that.

Mikep86

LGTM, thanks for the iterations!

kderusso added 4 commits July 25, 2024 16:06

Cleanup: Remove pinned IDs from applied rules in favor of single appl…

a581177

…ied docs

Add support for query rules of type exclude, to exclude specified doc…

f905e9c

…uments from result sets

Support exluded documents that specify the _index as well as the _id

0e575d1

Cleanup

1a89e1c

elasticsearchmachine added the v8.16.0 label Jul 29, 2024

kderusso added >feature :SearchOrg/Relevance Label for the Search (solution/org) Relevance team labels Jul 29, 2024

kderusso commented Jul 29, 2024

View reviewed changes

kderusso added 2 commits July 29, 2024 14:51

Update docs/changelog/111420.yaml

dd89c02

Update docs

d63b5e3

kderusso force-pushed the kderusso/query-rule-exclude branch from 10bc011 to d63b5e3 Compare July 29, 2024 18:51

Spotless

dc7186c

kderusso force-pushed the kderusso/query-rule-exclude branch from 315df19 to dc7186c Compare July 29, 2024 19:18

kderusso marked this pull request as ready for review July 29, 2024 20:24

kderusso requested a review from a team July 29, 2024 20:24

markjhoy reviewed Jul 29, 2024

View reviewed changes

PR feedback - docs updates

e3f2f36

Merge branch 'main' into kderusso/query-rule-exclude

4116f14

Mikep86 reviewed Jul 31, 2024

View reviewed changes

Apply PR feedback

36f6a64

kderusso requested review from markjhoy and Mikep86 August 1, 2024 15:43

Merge branch 'main' into kderusso/query-rule-exclude

2a4c75e

Samiul-TheSoccerFan reviewed Aug 1, 2024

View reviewed changes

...iness-rules/src/main/java/org/elasticsearch/xpack/searchbusinessrules/SpecifiedDocument.java Show resolved Hide resolved

Mikep86 reviewed Aug 1, 2024

View reviewed changes

PR feedback

49311e0

kderusso requested a review from Mikep86 August 1, 2024 19:56

Mikep86 approved these changes Aug 1, 2024

View reviewed changes

kderusso merged commit 02c4949 into elastic:main Aug 2, 2024
15 checks passed

kderusso deleted the kderusso/query-rule-exclude branch January 24, 2025 13:54

[Query rules] Add exclude query rule type #111420

[Query rules] Add exclude query rule type #111420

Uh oh!

Conversation

kderusso commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jul 29, 2024

Uh oh!

kderusso Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jul 29, 2024

Uh oh!

elasticsearchmachine commented Jul 29, 2024

Uh oh!

markjhoy Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

markjhoy Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

markjhoy Jul 29, 2024

Choose a reason for hiding this comment

Uh oh!

kderusso Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

kderusso commented Jul 30, 2024

Uh oh!

Mikep86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kderusso commented Aug 1, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mikep86 Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Mikep86 left a comment

Choose a reason for hiding this comment

Uh oh!

Mikep86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[Query rules] Add `exclude` query rule type #111420

[Query rules] Add `exclude` query rule type #111420

kderusso commented Jul 29, 2024 •

edited

Loading