address comments

Signed-off-by: Peng Huo <[email protected]>
opensearch-project · Jan 26, 2022 · 04517f5 · 04517f5
1 parent e9aed4d
commit 04517f5
Show file tree

Hide file tree

Showing 8 changed files with 20 additions and 17 deletions.
diff --git a/...src/main/java/org/opensearch/search/aggregations/bucket/composite/BinaryValuesSource.java b/...src/main/java/org/opensearch/search/aggregations/bucket/composite/BinaryValuesSource.java
@@ -105,7 +105,7 @@ void copyCurrent(int slot) {
     int compare(int from, int to) {
         if (missingBucket) {
             int result = missingOrder.compare(() -> Objects.isNull(values.get(from)), () -> Objects.isNull(values.get(to)), reverseMul);
-            if (!MissingOrder.unknownOrder(result)) {
+            if (MissingOrder.unknownOrder(result) == false) {
                 return result;
             }
         }
@@ -116,7 +116,7 @@ int compare(int from, int to) {
     int compareCurrent(int slot) {
         if (missingBucket) {
             int result = missingOrder.compare(() -> Objects.isNull(currentValue), () -> Objects.isNull(values.get(slot)), reverseMul);
-            if (!MissingOrder.unknownOrder(result)) {
+            if (MissingOrder.unknownOrder(result) == false) {
                 return result;
             }
         }
@@ -127,7 +127,7 @@ int compareCurrent(int slot) {
     int compareCurrentWithAfter() {
         if (missingBucket) {
             int result = missingOrder.compare(() -> Objects.isNull(currentValue), () -> Objects.isNull(afterValue), reverseMul);
-            if (!MissingOrder.unknownOrder(result)) {
+            if (MissingOrder.unknownOrder(result) == false) {
                 return result;
             }
         }

diff --git a/...ava/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java b/...ava/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java
@@ -131,8 +131,8 @@ public final XContentBuilder toXContent(XContentBuilder builder, Params params)
         if (format != null) {
             builder.field("format", format);
         }
-        if (!MissingOrder.isDefault(missingOrder)) {
-            builder.field("missing_order", missingOrder.toString());
+        if (MissingOrder.isDefault(missingOrder) == false) {
+            builder.field(MissingOrder.NAME, missingOrder.toString());
         }
         builder.field("order", order);
         doXContentBody(builder, params);
@@ -325,7 +325,7 @@ protected abstract CompositeValuesSourceConfig innerBuild(QueryShardContext quer
 
     public final CompositeValuesSourceConfig build(QueryShardContext queryShardContext) throws IOException {
         if (missingBucket == false && missingOrder != MissingOrder.DEFAULT) {
-            throw new IllegalArgumentException("missing_order required missing_bucket is true");
+            throw new IllegalArgumentException(MissingOrder.NAME + " require missing_bucket is true");
         }
         ValuesSourceConfig config = ValuesSourceConfig.resolve(
             queryShardContext,

diff --git a/...java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceConfig.java b/...java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceConfig.java
@@ -137,7 +137,7 @@ boolean missingBucket() {
     }
 
     /**
-     * If true, an explicit `null bucket represents documents with missing values.
+     * Return the {@link MissingOrder} for the config.
      */
     MissingOrder missingOrder() {
         return missingOrder;

diff --git a/...rg/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceParserHelper.java b/...rg/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceParserHelper.java
@@ -43,6 +43,7 @@
 import org.opensearch.common.xcontent.XContentBuilder;
 import org.opensearch.common.xcontent.XContentParser;
 import org.opensearch.script.Script;
+import org.opensearch.search.aggregations.bucket.missing.MissingOrder;
 import org.opensearch.search.aggregations.support.ValueType;
 
 import java.io.IOException;
@@ -54,7 +55,7 @@ public class CompositeValuesSourceParserHelper {
     static <VB extends CompositeValuesSourceBuilder<VB>, T> void declareValuesSourceFields(AbstractObjectParser<VB, T> objectParser) {
         objectParser.declareField(VB::field, XContentParser::text, new ParseField("field"), ObjectParser.ValueType.STRING);
         objectParser.declareBoolean(VB::missingBucket, new ParseField("missing_bucket"));
-        objectParser.declareString(VB::missingOrder, new ParseField("missing_order"));
+        objectParser.declareString(VB::missingOrder, new ParseField(MissingOrder.NAME));
 
         objectParser.declareField(VB::userValuetypeHint, p -> {
             ValueType valueType = ValueType.lenientParse(p.text());

diff --git a/...src/main/java/org/opensearch/search/aggregations/bucket/composite/DoubleValuesSource.java b/...src/main/java/org/opensearch/search/aggregations/bucket/composite/DoubleValuesSource.java
@@ -92,7 +92,7 @@ void copyCurrent(int slot) {
     int compare(int from, int to) {
         if (missingBucket) {
             int result = missingOrder.compare(() -> bits.get(from) == false, () -> bits.get(to) == false, reverseMul);
-            if (!MissingOrder.unknownOrder(result)) {
+            if (MissingOrder.unknownOrder(result) == false) {
                 return result;
             }
         }
@@ -103,7 +103,7 @@ int compare(int from, int to) {
     int compareCurrent(int slot) {
         if (missingBucket) {
             int result = missingOrder.compare(() -> missingCurrentValue, () -> bits.get(slot) == false, reverseMul);
-            if (!MissingOrder.unknownOrder(result)) {
+            if (MissingOrder.unknownOrder(result) == false) {
                 return result;
             }
         }
@@ -114,7 +114,7 @@ int compareCurrent(int slot) {
     int compareCurrentWithAfter() {
         if (missingBucket) {
             int result = missingOrder.compare(() -> missingCurrentValue, () -> afterValue == null, reverseMul);
-            if (!MissingOrder.unknownOrder(result)) {
+            if (MissingOrder.unknownOrder(result) == false) {
                 return result;
             }
         }

diff --git a/...n/java/org/opensearch/search/aggregations/bucket/composite/GlobalOrdinalValuesSource.java b/...n/java/org/opensearch/search/aggregations/bucket/composite/GlobalOrdinalValuesSource.java
@@ -91,7 +91,7 @@ void copyCurrent(int slot) {
     int compare(int from, int to) {
         if (missingBucket) {
             int result = missingOrder.compare(() -> values.get(from) == -1, () -> values.get(to) == -1, reverseMul);
-            if (!MissingOrder.unknownOrder(result)) {
+            if (MissingOrder.unknownOrder(result) == false) {
                 return result;
             }
         }
@@ -102,7 +102,7 @@ int compare(int from, int to) {
     int compareCurrent(int slot) {
         if (missingBucket) {
             int result = missingOrder.compare(() -> currentValue == -1, () -> values.get(slot) == -1, reverseMul);
-            if (!MissingOrder.unknownOrder(result)) {
+            if (MissingOrder.unknownOrder(result) == false) {
                 return result;
             }
         }
@@ -113,7 +113,7 @@ int compareCurrent(int slot) {
     int compareCurrentWithAfter() {
         if (missingBucket) {
             int result = missingOrder.compare(() -> currentValue == -1, () -> afterValueGlobalOrd == -1, reverseMul);
-            if (!MissingOrder.unknownOrder(result)) {
+            if (MissingOrder.unknownOrder(result) == false) {
                 return result;
             }
         }

diff --git a/...r/src/main/java/org/opensearch/search/aggregations/bucket/composite/LongValuesSource.java b/...r/src/main/java/org/opensearch/search/aggregations/bucket/composite/LongValuesSource.java
@@ -111,7 +111,7 @@ void copyCurrent(int slot) {
     int compare(int from, int to) {
         if (missingBucket) {
             int result = missingOrder.compare(() -> bits.get(from) == false, () -> bits.get(to) == false, reverseMul);
-            if (!MissingOrder.unknownOrder(result)) {
+            if (MissingOrder.unknownOrder(result) == false) {
                 return result;
             }
         }
@@ -122,7 +122,7 @@ int compare(int from, int to) {
     int compareCurrent(int slot) {
         if (missingBucket) {
             int result = missingOrder.compare(() -> missingCurrentValue, () -> bits.get(slot) == false, reverseMul);
-            if (!MissingOrder.unknownOrder(result)) {
+            if (MissingOrder.unknownOrder(result) == false) {
                 return result;
             }
         }
@@ -133,7 +133,7 @@ int compareCurrent(int slot) {
     int compareCurrentWithAfter() {
         if (missingBucket) {
             int result = missingOrder.compare(() -> missingCurrentValue, () -> Objects.isNull(afterValue), reverseMul);
-            if (!MissingOrder.unknownOrder(result)) {
+            if (MissingOrder.unknownOrder(result) == false) {
                 return result;
             }
         }

diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/missing/MissingOrder.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/missing/MissingOrder.java
@@ -80,6 +80,8 @@ public String toString() {
         }
     };
 
+    public static final String NAME = "missing_order";
+
     private static int MISSING_ORDER_UNKNOWN = Integer.MIN_VALUE;
 
     public static MissingOrder readFromStream(StreamInput in) throws IOException {
-Original file line number
+Diff line change
@@ Expand Up / @@ -80,6 +80,8 @@ public String toString() { @@
             }
         };
+        public static final String NAME = "missing_order";
         private static int MISSING_ORDER_UNKNOWN = Integer.MIN_VALUE;
         public static MissingOrder readFromStream(StreamInput in) throws IOException {
@@ Expand Down @@