Skip to content

Disallow kNN searches on nested vector fields #79403

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

Merged
merged 2 commits into from
Oct 20, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ public void validate(MappingLookup mappers) {
throw new MapperParsingException("Invalid [path] value [" + path + "] for field alias [" +
name() + "]: an alias cannot refer to another alias.");
}
String aliasScope = mappers.getNestedScope(name);
String pathScope = mappers.getNestedScope(path);
String aliasScope = mappers.getNestedParent(name);
String pathScope = mappers.getNestedParent(path);

if (Objects.equals(aliasScope, pathScope) == false) {
StringBuilder message = new StringBuilder("Invalid [path] value [" + path + "] for field alias [" +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ public final void validate(MappingLookup mappers) {
throw new IllegalArgumentException("[copy_to] may not be used to copy from a multi-field: [" + this.name() + "]");
}

final String sourceScope = mappers.getNestedScope(this.name());
final String sourceScope = mappers.getNestedParent(this.name());
for (String copyTo : this.copyTo().copyToFields()) {
if (mappers.isMultiField(copyTo)) {
throw new IllegalArgumentException("[copy_to] may not be used to copy to a multi-field: [" + copyTo + "]");
Expand All @@ -299,7 +299,7 @@ public final void validate(MappingLookup mappers) {
throw new IllegalArgumentException("Cannot copy to field [" + copyTo + "] since it is mapped as an object");
}

final String targetScope = mappers.getNestedScope(copyTo);
final String targetScope = mappers.getNestedParent(copyTo);
checkNestedScopeCompatibility(sourceScope, targetScope);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,14 @@ public boolean isObjectField(String field) {
return objectMappers.containsKey(field);
}

public String getNestedScope(String path) {
/**
* Given a nested object path, returns the path to its nested parent
*
* In particular, if a nested field `foo` contains an object field
* `bar.baz`, then calling this method with `foo.bar.baz` will return
* the path `foo`, skipping over the object-but-not-nested `foo.bar`
*/
public String getNestedParent(String path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems that the previous getNestedParent will return null if path is not an objectMapper? But from how this method was used, it looks like this check is not important. So this change LGTM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, this behavior is different but I also couldn't find any place where this check was important.

for (String parentPath = parentObject(path); parentPath != null; parentPath = parentObject(parentPath)) {
ObjectMapper objectMapper = objectMappers.get(parentPath);
if (objectMapper != null && objectMapper.isNested()) {
Expand Down Expand Up @@ -466,34 +473,4 @@ public List<NestedObjectMapper> getNestedParentMappers() {
}
return parents;
}

/**
* Given a nested object path, returns the path to its nested parent
*
* In particular, if a nested field `foo` contains an object field
* `bar.baz`, then calling this method with `foo.bar.baz` will return
* the path `foo`, skipping over the object-but-not-nested `foo.bar`
*/
public String getNestedParent(String path) {
ObjectMapper mapper = objectMappers().get(path);
if (mapper == null) {
return null;
}
if (path.contains(".") == false) {
return null;
}
do {
path = path.substring(0, path.lastIndexOf("."));
mapper = objectMappers().get(path);
if (mapper == null) {
return null;
}
if (mapper.isNested()) {
return path;
}
if (path.contains(".") == false) {
return null;
}
} while(true);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,26 @@ setup:
dims: 5
index: true
similarity: l2_norm
comments:
type: nested
properties:
body:
type: text
vector:
type: dense_vector
dims: 5
index: true
similarity: l2_norm

- do:
index:
index: test
body:
name: cow.jpg
vector: [230.0, 300.33, -34.8988, 15.555, -200.0]
comments:
- body: "free the cows"
vector: [0.75, 100.0, 0.33, 16.2, -10.2]

- do:
index:
Expand All @@ -28,6 +42,9 @@ setup:
body:
name: moose.jpg
vector: [-0.5, 100.0, -13, 14.8, -156.0]
comments:
- body: "what a great moose"
Copy link
Contributor

Choose a reason for hiding this comment

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

cool descriptions!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hehe, I got bored with my usual test examples :)

vector: [11.4, 99.0, 1.55, -2.9, -10.2]

- do:
index:
Expand Down Expand Up @@ -63,19 +80,20 @@ setup:
"Test nonexistent field":
- do:
catch: bad_request
search:
rest_total_hits_as_int: true
index: test-index
knn_search:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are extra changes here because I caught and fixed some mistakes in these REST tests.

index: test
body:
query:
knn:
field: nonexistent
query_vector: [ -0.5, 90.0, -10, 14.8, -156.0 ]
num_candidates: 1
- match: { error.root_cause.0.type: "illegal_argument_exception" }
fields: [ "name" ]
knn:
field: nonexistent
query_vector: [ -0.5, 90.0, -10, 14.8, -156.0 ]
k: 2
num_candidates: 3
- match: { error.root_cause.0.type: "query_shard_exception" }
- match: { error.root_cause.0.reason: "failed to create query: field [nonexistent] does not exist in the mapping" }

---
"Direct knn queries are disallowed":
"Direct kNN queries are disallowed":
- do:
catch: bad_request
search:
Expand All @@ -88,3 +106,20 @@ setup:
query_vector: [ -0.5, 90.0, -10, 14.8, -156.0 ]
num_candidates: 1
- match: { error.root_cause.0.type: "illegal_argument_exception" }
- match: { error.root_cause.0.reason: "[knn] queries cannot be provided directly, use the [_knn_search] endpoint instead" }

---
"kNN searches on nested fields are disallowed":
- do:
catch: bad_request
knn_search:
index: test
body:
fields: [ "nonexistent" ]
knn:
field: comments.vector
query_vector: [ -0.5, 90.0, -10, 14.8, -156.0 ]
k: 2
num_candidates: 3
- match: { error.root_cause.0.type: "query_shard_exception" }
- match: { error.root_cause.0.reason: "failed to create query: [knn] queries are not supported on nested fields" }
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,17 @@ protected Query doToQuery(SearchExecutionContext context) {
+ DenseVectorFieldMapper.CONTENT_TYPE + "] fields");
}

if (context.getNestedParent(fieldType.name()) != null) {
throw new IllegalArgumentException("[" + NAME + "] queries are not supported on nested fields");
}

DenseVectorFieldType vectorFieldType = (DenseVectorFieldType) fieldType;
if (queryVector.length != vectorFieldType.dims()) {
throw new IllegalArgumentException("the query vector has a different dimension [" + queryVector.length + "] "
+ "than the index vectors [" + vectorFieldType.dims() + "]");
}
if (vectorFieldType.isSearchable() == false) {
throw new IllegalArgumentException("[" + "[" + NAME + "] queries are not supported if [index] is disabled");
throw new IllegalArgumentException("[" + NAME + "] queries are not supported if [index] is disabled");
}
return new KnnVectorQuery(fieldType.name(), queryVector, numCands);
}
Expand Down