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.

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

[BUG] Fail to visit multi-nested for some QueryBuilder #13812

Open
wdongyu opened this issue May 24, 2024 · 1 comment
Open

[BUG] Fail to visit multi-nested for some QueryBuilder #13812

wdongyu opened this issue May 24, 2024 · 1 comment
Labels
bug Something isn't working Search:Query Capabilities

Comments

@wdongyu
Copy link

wdongyu commented May 24, 2024

Describe the bug

A way to visit all nested QueryBuilders from source have been implemented before, but it has some flaws.

For example, in BooleanQueryBuilder, the visit method first accept the current BooleanQueryBuilder, and then go down the tree by calling its nested must/should/filter QueryBuilder's visit methods. That's expected.

    public void visit(QueryBuilderVisitor visitor) {
        visitor.visit(this);
        if (mustClauses.isEmpty() == false) {
            QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST);
            for (QueryBuilder mustClause : mustClauses) {
                mustClause.visit(subVisitor);
            }
        }
        if (shouldClauses.isEmpty() == false) {
            QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.SHOULD);
            for (QueryBuilder shouldClause : shouldClauses) {
                shouldClause.visit(subVisitor);
            }
        }
        if (mustNotClauses.isEmpty() == false) {
            QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.MUST_NOT);
            for (QueryBuilder mustNotClause : mustNotClauses) {
                mustNotClause.visit(subVisitor);
            }
        }
        if (filterClauses.isEmpty() == false) {
            QueryBuilderVisitor subVisitor = visitor.getChildVisitor(Occur.FILTER);
            for (QueryBuilder filterClause : filterClauses) {
                filterClause.visit(subVisitor);
            }
        }
    }

But for BoostingQueryBuilder, the visit method also first accept the current BoostingQueryBuilder, but next accept the nest positive/negative QueryBuiler, instead of visiting them:

  public void visit(QueryBuilderVisitor visitor) {
      visitor.accept(this);
      if (positiveQuery != null) {
          visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(positiveQuery);
      }
      if (negativeQuery != null) {
          visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(negativeQuery);
      }
  }

We should go down the tree by calling the nested QueryBuilder's visit method, like this:

    public void visit(QueryBuilderVisitor visitor) {
        visitor.accept(this);
        if (positiveQuery != null) {
            positiveQuery.visit(visitor.getChildVisitor(BooleanClause.Occur.MUST));

            // visitor.getChildVisitor(BooleanClause.Occur.MUST).accept(positiveQuery);
        }
        if (negativeQuery != null) {
            negativeQuery.visit(visitor.getChildVisitor(BooleanClause.Occur.SHOULD));
          
            // visitor.getChildVisitor(BooleanClause.Occur.SHOULD).accept(negativeQuery);
        }
    }

Related component

Search:Query Capabilities

To Reproduce

change and run the following testcase in BoostingQueryBuilder, it will success but should fail in deed:

    public void testVisit() {
      BoostingQueryBuilder builder = new BoostingQueryBuilder(
          new BoolQueryBuilder()
              .must(new TermQueryBuilder("unmapped_field1", "value1"))
              .should(new TermQueryBuilder("unmapped_field2", "value2")),
            
          new TermQueryBuilder(KEYWORD_FIELD_NAME, "other_value")
      );

      List<QueryBuilder> visitedQueries = new ArrayList<>();
      builder.visit(createTestVisitor(visitedQueries));

        assertEquals(3, visitedQueries.size());
    }

Expected behavior

change and run the following testcase in BoostingQueryBuilder, it should success:

    public void testVisit() {
      BoostingQueryBuilder builder = new BoostingQueryBuilder(
          new BoolQueryBuilder()
              .must(new TermQueryBuilder("unmapped_field1", "value1"))
              .should(new TermQueryBuilder("unmapped_field2", "value2")),
            
          new TermQueryBuilder(KEYWORD_FIELD_NAME, "other_value")
      );

      List<QueryBuilder> visitedQueries = new ArrayList<>();
      builder.visit(createTestVisitor(visitedQueries));

        assertEquals(5, visitedQueries.size());  // change 3 -> 5
    }

Additional Details

Plugins
Please list all plugins currently enabled.

Screenshots
If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

  • OS: [e.g. iOS]
  • Version [e.g. 22]

Additional context
Add any other context about the problem here.

@peternied
Copy link
Member

[Triage - attendees 1 2 3 4 5 6
@wdongyu Thanks for creating this detailed issue and a pull request to address the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Search:Query Capabilities
Projects
Status: Later (6 months plus)
Development

No branches or pull requests

2 participants