From 24dfcdb0f6c5905ac24df6fc8d98f55622c8b06a Mon Sep 17 00:00:00 2001 From: Martin Gaievski Date: Wed, 6 Jul 2022 11:29:03 -0700 Subject: [PATCH] Refactoring code to address comments Signed-off-by: Martin Gaievski --- .../index/codec/KnnVectorFormatFactory.java | 6 +--- .../index/mapper/DenseVectorFieldMapper.java | 2 +- .../index/mapper/KnnAlgorithmContext.java | 34 ++++++++----------- .../opensearch/index/mapper/KnnContext.java | 7 ++-- .../translog/InternalTranslogManager.java | 12 +++---- .../index/translog/TranslogManager.java | 6 ++-- 6 files changed, 30 insertions(+), 37 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/codec/KnnVectorFormatFactory.java b/server/src/main/java/org/opensearch/index/codec/KnnVectorFormatFactory.java index 2cbc1d53392ac..e353fa2ca991a 100644 --- a/server/src/main/java/org/opensearch/index/codec/KnnVectorFormatFactory.java +++ b/server/src/main/java/org/opensearch/index/codec/KnnVectorFormatFactory.java @@ -8,7 +8,6 @@ import org.apache.lucene.codecs.KnnVectorsFormat; import org.apache.lucene.codecs.lucene92.Lucene92Codec; import org.apache.lucene.codecs.lucene92.Lucene92HnswVectorsFormat; -import org.opensearch.index.mapper.DenseVectorFieldMapper; import org.opensearch.index.mapper.KnnAlgorithmContext; import org.opensearch.index.mapper.MappedFieldType; import org.opensearch.index.mapper.MapperService; @@ -54,10 +53,7 @@ public KnnVectorsFormat create(final String field) { } private boolean isDenseVectorFieldType(final MappedFieldType mappedFieldType) { - if (mappedFieldType instanceof DenseVectorFieldMapper.DenseVectorFieldType) { - return true; - } - return false; + return mappedFieldType instanceof DenseVectorFieldType; } private int getIntegerParam(Map methodParams, String name) { diff --git a/server/src/main/java/org/opensearch/index/mapper/DenseVectorFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/DenseVectorFieldMapper.java index 7d41382f0191e..93dd360641ac6 100644 --- a/server/src/main/java/org/opensearch/index/mapper/DenseVectorFieldMapper.java +++ b/server/src/main/java/org/opensearch/index/mapper/DenseVectorFieldMapper.java @@ -41,7 +41,7 @@ public final class DenseVectorFieldMapper extends FieldMapper { /** * Define the max dimension a knn_vector mapping can have. */ - public static final int MAX_DIMENSION = 1024; + private static final int MAX_DIMENSION = 1024; private static DenseVectorFieldMapper toType(FieldMapper in) { return (DenseVectorFieldMapper) in; diff --git a/server/src/main/java/org/opensearch/index/mapper/KnnAlgorithmContext.java b/server/src/main/java/org/opensearch/index/mapper/KnnAlgorithmContext.java index 7f4246ce5ca87..b8cdd3a717628 100644 --- a/server/src/main/java/org/opensearch/index/mapper/KnnAlgorithmContext.java +++ b/server/src/main/java/org/opensearch/index/mapper/KnnAlgorithmContext.java @@ -25,14 +25,14 @@ public class KnnAlgorithmContext implements ToXContentFragment, Writeable { private static final String PARAMETERS = "parameters"; private static final String NAME = "name"; + private static final int MAX_NUMBER_OF_ALGORITHM_PARAMETERS = 50; + private final Method method; private final Map parameters; - private static final int MAX_NUMBER_OF_ALGORITHM_PARAMETERS = 50; - public KnnAlgorithmContext(Method method, Map parameters) { - this.method = method; - this.parameters = parameters; + this.method = Objects.requireNonNull(method, "[method] for knn algorithm context cannot be null"); + this.parameters = Objects.requireNonNull(parameters, "[parameters] for knn algorithm context cannot be null"); } public Method getMethod() { @@ -78,9 +78,7 @@ public static KnnAlgorithmContext parse(Object in) { parameters = ((Map) value).entrySet().stream().collect(Collectors.toMap(Map.Entry::getKey, e -> { Object v = e.getValue(); if (v instanceof Map) { - throw new MapperParsingException( - String.format(Locale.ROOT, "Unable to parse parameter [%s] for [algorithm]", e.getValue()) - ); + throw new MapperParsingException(String.format(Locale.ROOT, "Unable to parse parameter [%s] for [algorithm]", v)); } return v; })); @@ -105,19 +103,17 @@ public static KnnAlgorithmContext parse(Object in) { public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.field(NAME, method.name()); if (parameters == null) { - builder.field(PARAMETERS, (String) null); - } else { - builder.startObject(PARAMETERS); - parameters.forEach((key, value) -> { - try { - builder.field(key, value); - } catch (IOException ioe) { - throw new RuntimeException("Unable to generate xcontent for method component"); - } - - }); - builder.endObject(); + return builder.field(PARAMETERS, (String) null); } + builder.startObject(PARAMETERS); + parameters.forEach((key, value) -> { + try { + builder.field(key, value); + } catch (IOException ioe) { + throw new RuntimeException("Unable to generate xcontent for method component"); + } + }); + builder.endObject(); return builder; } diff --git a/server/src/main/java/org/opensearch/index/mapper/KnnContext.java b/server/src/main/java/org/opensearch/index/mapper/KnnContext.java index 106c7fcf2101e..3d67bd4c44604 100644 --- a/server/src/main/java/org/opensearch/index/mapper/KnnContext.java +++ b/server/src/main/java/org/opensearch/index/mapper/KnnContext.java @@ -22,13 +22,14 @@ */ public final class KnnContext implements ToXContentFragment, Writeable { - private final Metric metric; - private final KnnAlgorithmContext knnAlgorithmContext; private static final String KNN_METRIC_NAME = "metric"; private static final String ALGORITHM = "algorithm"; + private final Metric metric; + private final KnnAlgorithmContext knnAlgorithmContext; + KnnContext(final Metric metric, final KnnAlgorithmContext knnAlgorithmContext) { - this.metric = metric; + this.metric = Objects.requireNonNull(metric, "[metric] for knn context cannot be null"); this.knnAlgorithmContext = knnAlgorithmContext; } diff --git a/server/src/main/java/org/opensearch/index/translog/InternalTranslogManager.java b/server/src/main/java/org/opensearch/index/translog/InternalTranslogManager.java index c7fdf1e30e6a1..e5ffe799eb90b 100644 --- a/server/src/main/java/org/opensearch/index/translog/InternalTranslogManager.java +++ b/server/src/main/java/org/opensearch/index/translog/InternalTranslogManager.java @@ -285,9 +285,9 @@ public void ensureCanFlush() { /** * Reads operations from the translog - * @param location + * @param location the location in the translog * @return the translog operation - * @throws IOException + * @throws IOException if an {@link IOException} occurs while executing method */ @Override public Translog.Operation readOperation(Translog.Location location) throws IOException { @@ -296,9 +296,9 @@ public Translog.Operation readOperation(Translog.Location location) throws IOExc /** * Adds an operation to the translog - * @param operation + * @param operation the operation in the translog * @return the location in the translog - * @throws IOException + * @throws IOException if an {@link IOException} occurs while executing method */ @Override public Translog.Location add(Translog.Operation operation) throws IOException { @@ -396,8 +396,8 @@ public String getTranslogUUID() { /** * - * @param localCheckpointOfLastCommit - * @param flushThreshold + * @param localCheckpointOfLastCommit the localCheckpoint of last commit in the translog + * @param flushThreshold the flush threshold in the translog * @return if the translog should be flushed */ public boolean shouldPeriodicallyFlush(long localCheckpointOfLastCommit, long flushThreshold) { diff --git a/server/src/main/java/org/opensearch/index/translog/TranslogManager.java b/server/src/main/java/org/opensearch/index/translog/TranslogManager.java index f82434f40b06c..fec51d9aa9463 100644 --- a/server/src/main/java/org/opensearch/index/translog/TranslogManager.java +++ b/server/src/main/java/org/opensearch/index/translog/TranslogManager.java @@ -98,15 +98,15 @@ public interface TranslogManager { * Reads operations for the translog * @param location the location in the translog * @return the translog operation - * @throws IOException + * @throws IOException if an {@link IOException} occurs while executing method */ Translog.Operation readOperation(Translog.Location location) throws IOException; /** * Adds an operation to the translog - * @param operation + * @param operation translog operation * @return the location in the translog - * @throws IOException + * @throws IOException if an {@link IOException} occurs while executing method */ Translog.Location add(Translog.Operation operation) throws IOException;