Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rename rule query and add support for multiple rulesets #108831

Merged
merged 12 commits into from
May 22, 2024
Merged

Rename rule query and add support for multiple rulesets #108831

merged 12 commits into from
May 22, 2024

Conversation

kderusso
Copy link
Member

@kderusso kderusso commented May 20, 2024 •
edited
Loading

This PR renames rule_query to rule and adds support for multiple specified ruleset_ids. Existing rule_query and ruleset_id syntax will work for now but will be removed in a future release.

PR includes some tech debt, removing unused fields from the RuleQueryBuilder to avoid cleanup in a subequent transport version change.

Example of the updated rule query syntax:

GET /test-index1/_search
{
  "query": {
    "rule": {
      "organic": {
        "query_string": {
          "query": "puggles"
        }
      },
      "match_criteria": {
        "dept": "ops"
      },
      "ruleset_ids": ["test-ruleset", "another-test-ruleset"]
    }
  }
}

@kderusso kderusso added >enhancement :SearchOrg/Relevance Label for the Search (solution/org) Relevance team v8.15.0 labels May 20, 2024
@github-actions GitHub Actions
Copy link

Documentation preview:

@elasticsearchmachine
Copy link
Collaborator

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

kderusso
kderusso commented May 20, 2024
View reviewed changes
...gin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/RuleQueryBuilder.java Outdated
}

private static final ConstructingObjectParser<RuleQueryBuilder, Void> PARSER = new ConstructingObjectParser<>(NAME, a -> {
QueryBuilder organicQuery = (QueryBuilder) a[0];
@SuppressWarnings("unchecked")
Map<String, Object> matchCriteria = (Map<String, Object>) a[1];
String rulesetId = (String) a[2];
return new RuleQueryBuilder(organicQuery, matchCriteria, rulesetId);
@SuppressWarnings("unchecked")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As specified, this allows legacy ruleset_id queries to continue to work.

@kderusso kderusso marked this pull request as ready for review May 20, 2024 19:17
@kderusso kderusso requested a review from a team May 20, 2024 19:18
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

afoucret
afoucret reviewed May 21, 2024
View reviewed changes
Copy link
Contributor

@afoucret afoucret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great overall. 2 small comments.

...gin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/RuleQueryBuilder.java Outdated Show resolved Hide resolved
...ck/plugin/ent-search/src/main/java/org/elasticsearch/xpack/application/EnterpriseSearch.java Outdated Show resolved Hide resolved
ioanatia
ioanatia reviewed May 21, 2024
View reviewed changes
...gin/ent-search/src/main/java/org/elasticsearch/xpack/application/rules/RuleQueryBuilder.java
throw new IllegalArgumentException("rulesetIds must not be null or empty");
}

if (rulesetIds.size() > MAX_NUM_RULESETS) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

completely unrelated to your changes, I do wonder if there are other places we make these types of validations, like maximum number of elements, checking for empty arrays or for null values in arrays - maybe these could be part of PARSER.declareStringArray? WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do like having the Exceptions in the constructor for testing purposes, but we can certainly look at standardizing these errors more. I'd like to keep that in a separate tech debt/cleanup initiative if you're OK with that though.

ioanatia
ioanatia approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@ioanatia ioanatia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good - I had one comment about a missing test #108831 (comment) that you could add before merging the PR

@kderusso
Copy link
Member Author

@elasticmachine update branch

@kderusso kderusso merged commit 74d7010 into elastic:main May 22, 2024
15 checks passed
@kderusso kderusso mentioned this pull request Jun 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ioanatia ioanatia ioanatia approved these changes

@afoucret afoucret Awaiting requested review from afoucret

@jimczi jimczi Awaiting requested review from jimczi

Assignees
No one assigned
Labels
>enhancement :SearchOrg/Relevance Label for the Search (solution/org) Relevance team v8.15.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@kderusso @elasticsearchmachine @afoucret @jimczi @ioanatia @elasticmachine