- 
                Notifications
    
You must be signed in to change notification settings  - Fork 25.6k
 
Rewrite more queries to match_none when they target an unmapped field #132987
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
base: main
Are you sure you want to change the base?
Rewrite more queries to match_none when they target an unmapped field #132987
Conversation
dfd6605    to
    51b6f9f      
    Compare
  
    | 
           Pinging @elastic/es-search-relevance (Team:Search Relevance)  | 
    
| 
           Hi @javanna, I've created a changelog YAML for you.  | 
    
| 
               | 
          ||
| QueryBuilder queryBuilderForProcessing = queryBuilder.rewrite(new SearchExecutionContext(executionContext)); | ||
| SearchExecutionContext copy = new SearchExecutionContext(executionContext); | ||
| configureContext(copy, isMapUnmappedFieldAsText()); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configuring the context was missing, hence the configuration values were not available to the underlying query rewrite. This fixes it.
| public void testTooLongRegexInRegexpQuery() throws Exception { | ||
| createIndex("idx"); | ||
| indexRandom(true, prepareIndex("idx").setSource("{}", XContentType.JSON)); | ||
| indexRandom(true, prepareIndex("idx").setSource("num", "value")); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no more failure when targeting an unmapped field, the test needs fixing to target a mapped field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super familiar with this rewrite logic but I did some spot checking and looked through the original ask and PR; this lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What makes this confusing is that we have a default index.query.parse.allow_unmapped_fields which defaults to true, and then individual queries have ignore_unmapped (are all of them deprecated?).
With this change, I think ignore_unmapped: false is broken for every where that accepts an ignore_unmapped.
| 
           Hi @javanna, I've updated the changelog YAML for you.  | 
    
| protected QueryBuilder doIndexMetadataRewrite(QueryRewriteContext context) throws IOException { | ||
| MappedFieldType fieldType = context.getFieldType(fieldName); | ||
| if (fieldType == null) { | ||
| return new MatchNoneQueryBuilder("The \"" + getName() + "\" query was rewritten to a \"match_none\" query."); | ||
| } | ||
| return this; | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignoreUnmapped defined in the query should take precedence right? It has a default value of false for this query type.
There are a bunch of these (HasChildQueryBuilder, HasParentQueryBuilder, NestedQueryBuilder, GeoGridQueryBuilder,ParentIdQueryBuilder, and maybe more..but these are the ones I found quickly).
We have a specific metadata rewrite that allows us to optionally rewrite a query builder based on mappings. It is used in some places to handle unmapped fields, but not consistently. This PR adds the rewrite to match_none to more queries when they target an unmapped field.
Closes #97129
Closes #131037