Skip to content

Don't return a parent field if its type is not listed in the types parameter #123148

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Don't return a parent field if its type is not listed in the types parameter #123148

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5a320fc
Remove parent fields when its type is not requested
luizgpsantos Feb 3, 2025
c89aceb
Merge branch 'elastic:main' into main
luizgpsantos Feb 13, 2025
f252a5c
Merge branch 'elastic:main' into main
luizgpsantos Feb 21, 2025
91afc7f
Use optimal data structure to check for selected types
luizgpsantos Feb 24, 2025
f1f1496
Remove unused method
luizgpsantos Feb 24, 2025
4dc545d
Update docs/changelog/123148.yaml
luizgpsantos Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion server/src/main/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.elasticsearch.tasks.CancellableTask;

import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
Expand Down Expand Up @@ -214,7 +215,10 @@ static Map<String, IndexFieldCapabilities> retrieveFieldCaps(
null,
Map.of()
);
responseMap.put(parentField, fieldCap);

if (filter == null || Arrays.asList(types).contains(type)) {
Copy link
Member

@piergm piergm Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. We can convert the array to a ArrayList before the for loop (this should be safe since we are not changing the array content).
  2. 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 for Set.of(types) since it returns an unmodifiable set!

Copy link
Contributor Author

@luizgpsantos luizgpsantos Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

responseMap.put(parentField, fieldCap);
}
}
dotIndex = parentField.lastIndexOf('.');
}
Expand Down
105 changes: 105 additions & 0 deletions server/src/test/java/org/elasticsearch/action/fieldcaps/FieldCapabilitiesFilterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,111 @@ public void testFieldTypeFiltering() throws IOException {
assertNull(response.get("_index"));
}

public void testExcludeParentWhenFieldTypeFilteringDoNotIncludeItsType() throws IOException {
MapperService mapperService = createMapperService("""
{ "_doc" : {
"properties" : {
"field1" : { "type" : "keyword" },
"field2" : { "type" : "long" },
"field3" : { "type" : "text" },
"field4" : {
"type" : "object",
"properties" : {
"field5" : { "type" : "keyword" }
}
}
}
} }
""");
SearchExecutionContext sec = createSearchExecutionContext(mapperService);

Map<String, IndexFieldCapabilities> response = FieldCapabilitiesFetcher.retrieveFieldCaps(
sec,
s -> true,
Strings.EMPTY_ARRAY,
new String[] { "text", "keyword" },
FieldPredicate.ACCEPT_ALL,
getMockIndexShard(),
true
);
assertNotNull(response.get("field1"));
assertNull(response.get("field2"));
assertNotNull(response.get("field3"));
assertNull(response.get("field4"));
assertNotNull(response.get("field4.field5"));
assertNull(response.get("_index"));
}

public void testIncludeParentWhenFieldTypeFilteringIncludeItsType() throws IOException {
MapperService mapperService = createMapperService("""
{ "_doc" : {
"properties" : {
"field1" : { "type" : "keyword" },
"field2" : { "type" : "long" },
"field3" : { "type" : "text" },
"field4" : {
"type" : "object",
"properties" : {
"field5" : { "type" : "keyword" }
}
}
}
} }
""");
SearchExecutionContext sec = createSearchExecutionContext(mapperService);

Map<String, IndexFieldCapabilities> response = FieldCapabilitiesFetcher.retrieveFieldCaps(
sec,
s -> true,
Strings.EMPTY_ARRAY,
new String[] { "text", "keyword", "object" },
FieldPredicate.ACCEPT_ALL,
getMockIndexShard(),
true
);
assertNotNull(response.get("field1"));
assertNull(response.get("field2"));
assertNotNull(response.get("field3"));
assertNotNull(response.get("field4"));
assertNotNull(response.get("field4.field5"));
assertNull(response.get("_index"));
}

public void testIncludeAllFieldsWhenNoFieldTypeFiltering() throws IOException {
MapperService mapperService = createMapperService("""
{ "_doc" : {
"properties" : {
"field1" : { "type" : "keyword" },
"field2" : { "type" : "long" },
"field3" : { "type" : "text" },
"field4" : {
"type" : "object",
"properties" : {
"field5" : { "type" : "keyword" }
}
}
}
} }
""");
SearchExecutionContext sec = createSearchExecutionContext(mapperService);

Map<String, IndexFieldCapabilities> response = FieldCapabilitiesFetcher.retrieveFieldCaps(
sec,
s -> true,
Strings.EMPTY_ARRAY,
Strings.EMPTY_ARRAY,
FieldPredicate.ACCEPT_ALL,
getMockIndexShard(),
true
);
assertNotNull(response.get("field1"));
assertNotNull(response.get("field2"));
assertNotNull(response.get("field3"));
assertNotNull(response.get("field4"));
assertNotNull(response.get("field4.field5"));
assertNotNull(response.get("_index"));
}

Copy link
Member

@piergm piergm Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@luizgpsantos luizgpsantos Feb 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private IndexShard getMockIndexShard() {
IndexShard indexShard = mock(IndexShard.class);
when(indexShard.getFieldInfos()).thenReturn(FieldInfos.EMPTY);
Expand Down