Don't return a parent field if its type is not listed in the types parameter #123148
Conversation
luizgpsantos
added
>enhancement
Team:Search Foundations
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
elasticsearchmachine
added
the
external-contributor
There was a problem hiding this comment.
The PR looks good @luizgpsantos! I left few comments but we are in the right track!
Thanks!
| @@ -214,7 +215,10 @@ static Map<String, IndexFieldCapabilities> retrieveFieldCaps( | |||
| null, | |||
| Map.of() | |||
| ); | |||
| responseMap.put(parentField, fieldCap); | |||
|
|
|||
| if (filter == null || Arrays.asList(types).contains(type)) { | |||
There was a problem hiding this comment.
Careful, by calling Arrays.asList in a loop, here we are converting every time the array to a new ArrayList and it is not very efficient.
There are a couple of options here:
- We can convert the array to a ArrayList before the for loop (this should be safe since we are not changing the array content).
- Use a Set instead of an ArrayList (also placed outside the loop), in this way the .contains call is O(1), while for ArrayList is O(n).
Side note: I don't expect this array to ever become huge therefore both approach should be safe to use but to be extra safe I'd opt for the Set.
If we are using Set we can even go forSet.of(types)since it returns an unmodifiable set!
There was a problem hiding this comment.
Yep, that makes sense. To avoid the duplicated initialization of Set.of(types), I changed some other places. Also, now I have to check for the parent in the filters, so I used the same Set strategy.
| assertNotNull(response.get("field4.field5")); | ||
| assertNotNull(response.get("_index")); | ||
| } | ||
|
|
There was a problem hiding this comment.
How should this work if we use the filters -parent or +parent? (ref).
Maybe we can add some more test coverage to make sure we have the expected behaviour here!
There was a problem hiding this comment.
I added two extra tests to cover that. What surprised me is that +parent doesn't exist as I would expect. Instead, parent is the affirmative form to retrieve the parent fields.
|
@elasticmachine test this please |
|
Drive-by comment: I don't understand in general why it is useful to return objects entirely in the field caps output. What capabilities do objects have? They are just container and are not so useful in terms of querying and aggregating on them? |
javanna
changed the title
|
Hi @luizgpsantos, I've created a changelog YAML for you. |
When setting the types query parameter in a field capability request, we aim to return only fields with the specified types. However, we currently return object fields if an object has at least one child of the requested type. This PR verifies whether the parent field type is included in the
typesparameters before adding it to the list of fields to be returned.fixes: #109797