Added some more tests

Signed-off-by: Norman Jordan <[email protected]>
opensearch-project · Nov 8, 2024 · a0f9c2c · a0f9c2c
1 parent 7d17615
commit a0f9c2c
Show file tree

Hide file tree

Showing 8 changed files with 102 additions and 25 deletions.
diff --git a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java
@@ -559,7 +559,7 @@ public LogicalPlan visitAD(AD node, AnalysisContext context) {
     return new LogicalAD(child, options);
   }
 
-  /** Build {@link LogicalAD} for fillnull command. */
+  /** Build {@link LogicalEval} for fillnull command. */
   @Override
   public LogicalPlan visitFillNull(final FillNull node, final AnalysisContext context) {
     LogicalPlan child = node.getChild().get(0).accept(this, context);

diff --git a/core/src/main/java/org/opensearch/sql/ast/tree/FillNull.java b/core/src/main/java/org/opensearch/sql/ast/tree/FillNull.java
@@ -16,6 +16,7 @@
 import org.opensearch.sql.ast.expression.Field;
 import org.opensearch.sql.ast.expression.UnresolvedExpression;
 
+/** AST node represent FillNull operation. */
 @RequiredArgsConstructor
 @AllArgsConstructor
 public class FillNull extends UnresolvedPlan {

diff --git a/docs/category.json b/docs/category.json
@@ -14,6 +14,7 @@
     "user/ppl/cmd/information_schema.rst",
     "user/ppl/cmd/eval.rst",
     "user/ppl/cmd/fields.rst",
+    "user/ppl/cmd/fillnull.rst",
     "user/ppl/cmd/grok.rst",
     "user/ppl/cmd/head.rst",
     "user/ppl/cmd/parse.rst",

diff --git a/docs/user/ppl/cmd/fillnull.rst b/docs/user/ppl/cmd/fillnull.rst
@@ -33,16 +33,16 @@ The example show to replace null values for email and host with "<not found>".
 
 PPL query::
 
-    os> source=accounts | fields email, host | fillnull with '<not found>' email, host ;
+    os> source=accounts | fields email, employer | fillnull with '<not found>' in email ;
     fetched rows / total rows = 4/4
-    +-----------------------+------------+
-    | email                 | host       |
-    |-----------------------+------------|
-    | [email protected]  | pyrami.com |
-    | [email protected] | netagy.com |
-    | <not found>           |            |
-    | [email protected]   | boink.com  |
-    +-----------------------+------------+
+    +-----------------------+----------+
+    | email                 | employer |
+    |-----------------------+----------|
+    | [email protected]  | Pyrami   |
+    | [email protected] | Netagy   |
+    | <not found>           | Quility  |
+    | [email protected]   | null     |
+    +-----------------------+----------+
 
 Example 2: Replace null values for multiple fields with different values
 ========================================================================

diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/ExplainIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/ExplainIT.java
@@ -97,8 +97,7 @@ public void testFillNullPushDownExplain() throws Exception {
         expected,
         explainQueryToString(
             "source=opensearch-sql_test_index_account"
-                + "| fields age, balance "
-                + "| fillnull with -1 in age,balance"));
+                + " | fillnull with -1 in age,balance | fields age, balance"));
   }
 
   String loadFromFile(String filename) throws Exception {

diff --git a/integ-test/src/test/java/org/opensearch/sql/ppl/FillNullCommandIT.java b/integ-test/src/test/java/org/opensearch/sql/ppl/FillNullCommandIT.java
@@ -127,4 +127,88 @@ public void testFillNullVariousValuesTwoFields() throws IOException {
         rows(-1, 10.98),
         rows(-1, 7.87));
   }
+
+  @Test
+  public void testFillNullWithOtherField() throws IOException {
+    JSONObject result =
+        executeQuery(
+            String.format(
+                "source=%s | fillnull using num0 = num1 | fields str2, num0", TEST_INDEX_CALCS));
+    verifyDataRows(
+        result,
+        rows("one", 12.3),
+        rows("two", -12.3),
+        rows("three", 15.7),
+        rows(null, -15.7),
+        rows("five", 3.5),
+        rows("six", -3.5),
+        rows(null, 0),
+        rows("eight", 11.38),
+        rows("nine", 10),
+        rows("ten", 12.4),
+        rows("eleven", 10.32),
+        rows("twelve", 2.47),
+        rows(null, 12.05),
+        rows("fourteen", 10.37),
+        rows("fifteen", 7.1),
+        rows("sixteen", 16.81),
+        rows(null, 7.12));
+  }
+
+  @Test
+  public void testFillNullWithFunctionOnOtherField() throws IOException {
+    JSONObject result =
+        executeQuery(
+            String.format(
+                "source=%s | fillnull with ceil(num1) in num0 | fields str2, num0",
+                TEST_INDEX_CALCS));
+    verifyDataRows(
+        result,
+        rows("one", 12.3),
+        rows("two", -12.3),
+        rows("three", 15.7),
+        rows(null, -15.7),
+        rows("five", 3.5),
+        rows("six", -3.5),
+        rows(null, 0),
+        rows("eight", 12),
+        rows("nine", 10),
+        rows("ten", 13),
+        rows("eleven", 11),
+        rows("twelve", 3),
+        rows(null, 13),
+        rows("fourteen", 11),
+        rows("fifteen", 8),
+        rows("sixteen", 17),
+        rows(null, 8));
+  }
+
+  @Test
+  public void testFillNullWithFunctionMultipleCommands() throws IOException {
+    JSONObject result =
+        executeQuery(
+            String.format(
+                "source=%s | fillnull with num1 in num0 | fields str2, num0 | fillnull with"
+                    + " 'unknown' in str2",
+                TEST_INDEX_CALCS));
+    verifyDataRows(
+        result,
+        rows("one", 12.3),
+        rows("two", -12.3),
+        rows("three", 15.7),
+        rows("unknown", -15.7),
+        rows("five", 3.5),
+        rows("six", -3.5),
+        rows("unknown", 0),
+        rows("eight", 11.38),
+        rows("nine", 10),
+        rows("ten", 12.4),
+        rows("eleven", 10.32),
+        rows("twelve", 2.47),
+        rows("unknown", 12.05),
+        rows("fourteen", 10.37),
+        rows("fifteen", 7.1),
+        rows("sixteen", 16.81),
+        rows("unknown", 7.12));
+  }
 }
diff --git a/integ-test/src/test/resources/expectedOutput/ppl/explain_fillnull_push.json b/integ-test/src/test/resources/expectedOutput/ppl/explain_fillnull_push.json
@@ -15,22 +15,14 @@
         },
         "children": [
           {
-            "name": "ProjectOperator",
+            "name": "OpenSearchIndexScan",
             "description": {
-              "fields": "[age, balance]"
+              "request": "OpenSearchQueryRequest(indexName=opensearch-sql_test_index_account, sourceBuilder={\"from\":0,\"size\":10000,\"timeout\":\"1m\"}, needClean=true, searchDone=false, pitId=null, cursorKeepAlive=null, searchAfter=null, searchResponse=null)"
             },
-            "children": [
-              {
-                "name": "OpenSearchIndexScan",
-                "description": {
-                  "request": "OpenSearchQueryRequest(indexName=opensearch-sql_test_index_account, sourceBuilder={\"from\":0,\"size\":10000,\"timeout\":\"1m\",\"_source\":{\"includes\":[\"age\",\"balance\"],\"excludes\":[]}}, needClean=true, searchDone=false, pitId=null, cursorKeepAlive=null, searchAfter=null, searchResponse=null)"
-                },
-                "children": []
-              }
-            ]
+            "children": []
           }
         ]
       }
     ]
   }
-}
+}
diff --git a/ppl/src/main/antlr/OpenSearchPPLParser.g4 b/ppl/src/main/antlr/OpenSearchPPLParser.g4
@@ -146,7 +146,7 @@ nullableField
    ;
 
 nullReplacement
-   : expression
+   : valueExpression
    ;
 
 kmeansCommand