Generalize how queries on _index are handled at rewrite time

[jpountz]

#49713 aims at introducing a new constant_keyword field which, like _index, always rewrites queries to a MatchAllQueryBuilder or a MatchNoneQueryBuilder in order to skip shards in the can_match phase. This change introduces a new ConstantFieldType marker class that helps get this functionality with any field and not just _index.

Since this change refactors rewrites, I also took it as an opportunity to adrress #49254: instead of returning the same queries you would get on a keyword field when a field is unmapped, queries get rewritten to a MatchNoDocsQueryBuilder.

This change exposed a couple bugs, like the fact that the percolator doesn't rewrite queries at query time, or that the significant_terms aggregation doesn't rewrite its inner filter, which I fixed.

Closes #49254

[elasticmachine]

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

[jpountz]

@elasticmachine run elasticsearch-ci/2

[mayya-sharipova]

thanks @jpountz, makes sense

[jtibshirani]

This also looks good to me, I had one suggestion I wanted to raise but don't feel strongly about. Currently the contract for classes implementing ConstantFieldType is that they should produce either MatchNoDocsQuery or MatchAllDocsQuery for certain query factory methods. They should also make sure to break out of termsQuery early if they find a match. To me this strategy felt more a bit more robust:

• Add abstract methods like bool matchesExact(Object value, QueryShardContext context) to ConstantFieldType.
• Define final versions of termQuery, termsQuery, etc. in ConstantFieldType that call into these methods.

[jtibshirani]

Small comment, could just be if (context != null && context.fieldMapper(...)) { ... }

[jtibshirani]

This looks unused, I think it can just be deleted?

[jtibshirani]

For my knowledge, I'm wondering why this change (and the ones to AbstractQueryTestCase) were necessary.

[jpountz]

I made this change as a way to keep tests simple. For instance here is what ConstantScoreQueryBuilderTests#doAssertLuceneQuery looks like in master today.

    @Override
    protected void doAssertLuceneQuery(ConstantScoreQueryBuilder queryBuilder, Query query, QueryShardContext context) throws IOException {
        Query innerQuery = queryBuilder.innerQuery().toQuery(context);
        if (innerQuery == null) {
            assertThat(query, nullValue());
        } else {
            assertThat(query, instanceOf(ConstantScoreQuery.class));
            ConstantScoreQuery constantScoreQuery = (ConstantScoreQuery) query;
            assertThat(constantScoreQuery.getQuery(), instanceOf(innerQuery.getClass()));
        }
    }

This test only worked because most queries on unmapped fields would create the same query as on a keyword field. But with this change, queries on unmapped fields get rewritten as a MatchNoneQueryBuilder. And when its inner query rewrites to a MatchNoneQueryBuilder, ConstantScoreQueryBuilder itself rewrites to a MatchNoneQueryBuilder. So I updated the logic this way:

    @Override
    protected void doAssertLuceneQuery(ConstantScoreQueryBuilder queryBuilder, Query query, QueryShardContext context) throws IOException {
        Query innerQuery = queryBuilder.innerQuery().rewrite(context).toQuery(context);
        if (innerQuery == null) {
            assertThat(query, nullValue());
        } else if (innerQuery instanceof MatchNoDocsQuery) {
            assertThat(query, instanceOf(MatchNoDocsQuery.class));
        } else {
            assertThat(query, instanceOf(ConstantScoreQuery.class));
            ConstantScoreQuery constantScoreQuery = (ConstantScoreQuery) query;
            assertThat(constantScoreQuery.getQuery(), instanceOf(innerQuery.getClass()));
        }
    }

But this fails if the inner query is wrapped in a BoostQuery. So I had to either change AbstractQueryBuilder to no longer wrap MatchNoDocsQuery with a boost, or change this test (and a couple other ones IIRC) to check whether the inner query might be wrapped inside a BoostQuery. I chose the former, which sounded simpler to me.

[jtibshirani]

Thanks for the clear explanation!

[jpountz]

@jtibshirani Thanks for your comments, I wondered whether I should do something like that and leaned towards not doing it to avoid introducing new abstractions, but I also agree that it reduces chances to not implement the contract. I'll implement your suggestion.