Refactoring of Indices Query

[alexksikes]

Relates to #10217

This PR is against the query-refactoring branch.

[cbuescher]

Not sure, but maybe we can add null-check for innerQuery here like in other nested query builders (e.g. BoostingQueryBuilder)? That would avoid potential NPEs or null checks in doXContent later.

[alexksikes]

sure i'll add that

[cbuescher]

Left a few comments, however I'm not sure how we should make the clusterService accessible in the builder. I think we need it there, but not sure how to make it accessible. For now using QueryParseContext and inject it there might be okay, on the long run we might need a similar context object but not for parsing but for the toQuery() step.

[alexksikes]

I agree this is something we need to discuss. This is more temporary until we figure this out.

[javanna]

I think this public constant is a bit scary, as it's an object and can be modified (think queryName or boost). I think in this case I would not add a constant for the default value, create a new query all the time.

[alexksikes]

sure i'll do that

[javanna]

hey @alexksikes I finally got back to this, did a review round and played around with this as well. I think it is not far, could you please rebase it and update it? I would love to get it in soon, refactoring this query will mean some more cleanups that can be done afterwards in QueryParseContext I think. Thanks!

[javanna]

btw you should stumble upon a test in SearchQueryIT that will fail (testIndicesQuerySkipParsing). I think that it cannot run successfully till we have rafactored nested queries, hence we need to mark @AwaitsFix and make sure we don't forget to get back to it.

[alexksikes]

@javanna I addressed all the comments and rebased. Thanks for the review.

[javanna]

can you make this private? then the comment is not even needed anymore I guess

[alexksikes]

sure

[javanna]

same here

[javanna]

I did another round and left a few comments. I think we are also missing some documentation for the new introduced query, unless we decide to not expose it to users, which we are doing at the moment though. @clintongormley do you have an opinion about this? Do you think a match_none query would be useful to users or shall we keep it internal only?

[javanna]

I don't think we need this here. very similar to discussion here: #12037 (comment) . it is needed because you use lucene equals to compare queries and because the boost from the main query overrides the inner boosts.

[javanna]

To be clear, the difference between indices query and wrapper query is that the latter doesn't support boost nor query name, while the former does. Saying that the query doesn't support boost and query name is just a way to work around an issue and skip some test checks that should work instead.

[alexksikes]

@javanna I addressed the comments. I am not exactly sure how best to inject the indices to the test cluster service, if you have any ideas?

[javanna]

maybe have a static package private method that returns the default recreating a new match_all all the time. That could be used from the parser and below in the setter too?

[clintongormley]

Do you think a match_none query would be useful to users or shall we keep it internal only?

I think it is for internal use only

[javanna]

I think we have to remove this given that we just decided to leave the match_none query for internal use only.

[alexksikes]

Yes we can remove that one. We just make the usage of this builder undocumented.

[alexksikes]

Given that the builder API now takes a QueryBuilder for the noMatch query, I would think we can't make it internal. So it would have to be public, and used for the sole purpose of being able to specify a query that matches none?

[javanna]

Given that the builder API now takes a QueryBuilder for the noMatch query, I would think we can't make it internal. So it would have to be public, and used for the sole purpose of being able to specify a query that matches none?

Here is how we can make that query internal: #12031 (comment) . People can still use if from the java api if they want to as the class is public, but it will not be exposed as other regular queries as it has no parser etc.

[javanna]

once #12937 is in we can do the following here:

        String[] indices;
        if (randomBoolean()) {
            indices = new String[]{getIndex().getName()};
        } else {
            indices = generateRandomStringArray(5, 10, false, false);
        }
        IndicesQueryBuilder query = new IndicesQueryBuilder(RandomQueryBuilder.createQuery(random()), indices);

[javanna]

I did another round of review and opened #12937 to expand testing here.

[alexksikes]

OK then once we get #12937 in, I'll address all the comments and rebase. Thanks.

[javanna]

#12937 is in, you can go ahead and rebase.

[alexksikes]

@javanna rebased and all comments addressed. I think it should be ready. Thanks for the review.

[javanna]

LGTM