Skip to content

Commit

Permalink
[Backport 1.x] Allow null value for params in method mappings (#355)
Browse files Browse the repository at this point in the history 
Allow user to input null value for parameters field for KNNMethodContext
and MethodComponentContext. Adds tests to reproduce Mapping Parsing Error when the parameters take
the value null as well as BWC tests.

Signed-off-by: John Mazanec <[email protected]>
(cherry picked from commit b08127c)
  • Loading branch information
@opensearch-trigger-bot
opensearch-trigger-bot[bot] authored Apr 7, 2022
1 parent bb42bb6 commit f2d0ebb
Show file tree
Hide file tree
Showing 5 changed files with 367 additions and 239 deletions.
36 changes: 21 additions & 15 deletions src/main/java/org/opensearch/knn/index/KNNMethodContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,17 @@
*/
public class KNNMethodContext implements ToXContentFragment, Writeable {

private static Logger logger = LogManager.getLogger(KNNMethodContext.class);
private static final Logger logger = LogManager.getLogger(KNNMethodContext.class);

private static KNNMethodContext defaultInstance = null;

public static synchronized KNNMethodContext getDefault() {
if (defaultInstance == null) {
defaultInstance = new KNNMethodContext(KNNEngine.DEFAULT, SpaceType.DEFAULT,
new MethodComponentContext(METHOD_HNSW, Collections.emptyMap()));
defaultInstance = new KNNMethodContext(
KNNEngine.DEFAULT,
SpaceType.DEFAULT,
new MethodComponentContext(METHOD_HNSW, Collections.emptyMap())
);
}
return defaultInstance;
}
Expand Down Expand Up @@ -191,21 +194,26 @@ public static KNNMethodContext parse(Object in) {

name = (String) value;
} else if (PARAMETERS.equals(key)) {
if (value == null) {
parameters = null;
continue;
}

if (!(value instanceof Map)) {
throw new MapperParsingException("Unable to parse parameters for main method component");
}

// Interpret all map parameters as sub-MethodComponentContexts
@SuppressWarnings("unchecked")
Map<String, Object> parameters1 = ((Map<String, Object>) value).entrySet().stream().collect(Collectors.toMap(
Map.Entry::getKey, e -> {
Object v = e.getValue();
if (v instanceof Map) {
return MethodComponentContext.parse(v);
}
return v;
Map<String, Object> parameters1 = ((Map<String, Object>) value).entrySet()
.stream()
.collect(Collectors.toMap(Map.Entry::getKey, e -> {
Object v = e.getValue();
if (v instanceof Map) {
return MethodComponentContext.parse(v);
}
));
return v;
}));

parameters = parameters1;
} else {
Expand All @@ -232,10 +240,8 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws

@Override
public boolean equals(Object obj) {
if (this == obj)
return true;
if (obj == null || getClass() != obj.getClass())
return false;
if (this == obj) return true;
if (obj == null || getClass() != obj.getClass()) return false;
KNNMethodContext other = (KNNMethodContext) obj;

EqualsBuilder equalsBuilder = new EqualsBuilder();
Expand Down
95 changes: 63 additions & 32 deletions src/main/java/org/opensearch/knn/index/MethodComponentContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.opensearch.index.mapper.MapperParsingException;

import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.stream.Collectors;
Expand All @@ -30,18 +31,17 @@
import static org.opensearch.knn.common.KNNConstants.NAME;
import static org.opensearch.knn.common.KNNConstants.PARAMETERS;


/**
* MethodComponentContext represents a single user provided building block of a knn library index.
*
* Each component is composed of a name and a map of parameters.
*/
public class MethodComponentContext implements ToXContentFragment, Writeable {

private static Logger logger = LogManager.getLogger(MethodComponentContext.class);
private static final Logger logger = LogManager.getLogger(MethodComponentContext.class);

private String name;
private Map<String, Object> parameters;
private final String name;
private final Map<String, Object> parameters;

/**
* Constructor
Expand All @@ -62,7 +62,15 @@ public MethodComponentContext(String name, Map<String, Object> parameters) {
*/
public MethodComponentContext(StreamInput in) throws IOException {
this.name = in.readString();
this.parameters = in.readMap(StreamInput::readString, new ParameterMapValueReader());

// Due to backwards compatibility issue, parameters could be null. To prevent any null pointer exceptions,
// do not read if their are no bytes left is null. Make sure this is in sync with the fellow read method. For
// more information, refer to https://github.com/opensearch-project/k-NN/issues/353.
if (in.available() > 0) {
this.parameters = in.readMap(StreamInput::readString, new ParameterMapValueReader());
} else {
this.parameters = null;
}
}

/**
Expand Down Expand Up @@ -94,21 +102,26 @@ public static MethodComponentContext parse(Object in) {
}
name = (String) value;
} else if (PARAMETERS.equals(key)) {
if (value == null) {
parameters = null;
continue;
}

if (!(value instanceof Map)) {
throw new MapperParsingException("Unable to parse parameters for method component");
}

// Check to interpret map parameters as sub-methodComponentContexts
@SuppressWarnings("unchecked")
Map<String, Object> parameters1 = ((Map<String, Object>) value).entrySet().stream().collect(Collectors.toMap(
Map.Entry::getKey, e -> {
Object v = e.getValue();
if (v instanceof Map) {
return MethodComponentContext.parse(v);
}
return v;
Map<String, Object> parameters1 = ((Map<String, Object>) value).entrySet()
.stream()
.collect(Collectors.toMap(Map.Entry::getKey, e -> {
Object v = e.getValue();
if (v instanceof Map) {
return MethodComponentContext.parse(v);
}
));
return v;
}));

parameters = parameters1;
} else {
Expand All @@ -126,31 +139,37 @@ public static MethodComponentContext parse(Object in) {
@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.field(NAME, name);
builder.startObject(PARAMETERS);
parameters.forEach((key, value) -> {
try {
if (value instanceof MethodComponentContext) {
builder.startObject(key);
((MethodComponentContext) value).toXContent(builder, params);
builder.endObject();
} else {
builder.field(key, value);
// Due to backwards compatibility issue, parameters could be null. To prevent any null pointer exceptions,
// we just create the null field. If parameters are not null, we created a nested structure. For more
// information, refer to https://github.com/opensearch-project/k-NN/issues/353.
if (parameters == null) {
builder.field(PARAMETERS, (String) null);
} else {
builder.startObject(PARAMETERS);
parameters.forEach((key, value) -> {
try {
if (value instanceof MethodComponentContext) {
builder.startObject(key);
((MethodComponentContext) value).toXContent(builder, params);
builder.endObject();
} else {
builder.field(key, value);
}
} catch (IOException ioe) {
throw new RuntimeException("Unable to generate xcontent for method component");
}
} catch (IOException ioe) {
throw new RuntimeException("Unable to generate xcontent for method component");
}

});
builder.endObject();
});
builder.endObject();
}

return builder;
}

@Override
public boolean equals(Object obj) {
if (this == obj)
return true;
if (obj == null || getClass() != obj.getClass())
return false;
if (this == obj) return true;
if (obj == null || getClass() != obj.getClass()) return false;
MethodComponentContext other = (MethodComponentContext) obj;

EqualsBuilder equalsBuilder = new EqualsBuilder();
Expand Down Expand Up @@ -179,13 +198,25 @@ public String getName() {
* @return parameters
*/
public Map<String, Object> getParameters() {
// Due to backwards compatibility issue, parameters could be null. To prevent any null pointer exceptions,
// return an empty map if parameters is null. For more information, refer to
// https://github.com/opensearch-project/k-NN/issues/353.
if (parameters == null) {
return Collections.emptyMap();
}
return parameters;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeString(this.name);
out.writeMap(this.parameters, StreamOutput::writeString, new ParameterMapValueWriter());

// Due to backwards compatibility issue, parameters could be null. To prevent any null pointer exceptions,
// do not write if parameters is null. Make sure this is in sync with the fellow read method. For more
// information, refer to https://github.com/opensearch-project/k-NN/issues/353.
if (this.parameters != null) {
out.writeMap(this.parameters, StreamOutput::writeString, new ParameterMapValueWriter());
}
}

// Because the generic StreamOutput writeMap method can only write generic values, we need to create a custom one
Expand Down
Loading

0 comments on commit f2d0ebb

Please sign in to comment.