PPL fillnull command enhancement (#4421)

* PPL fillnull command enhancement

Signed-off-by: Kai Huang <[email protected]>

# Conflicts:
#	integ-test/src/test/java/org/opensearch/sql/calcite/CalciteNoPushdownIT.java

* add to searchableKeyWord

Signed-off-by: Kai Huang <[email protected]>

* fixes

Signed-off-by: Kai Huang <[email protected]>

* fix CI

Signed-off-by: Kai Huang <[email protected]>

* update error message handling

Signed-off-by: Kai Huang <[email protected]>

* formatting

Signed-off-by: Kai Huang <[email protected]>

* put file back

Signed-off-by: Kai Huang <[email protected]>

* removal

Signed-off-by: Kai Huang <[email protected]>

* update doc

Signed-off-by: Kai Huang <[email protected]>

* update doc

Signed-off-by: Kai Huang <[email protected]>

* add IT

Signed-off-by: Kai Huang <[email protected]>

---------

Signed-off-by: Kai Huang <[email protected]>

Author: ahkcs

core/src/main/java/org/opensearch/sql/ast/dsl/AstDSL.java

@@ -596,11 +596,25 @@ public static FillNull fillNull(UnresolvedPlan input, UnresolvedExpression repla
     return FillNull.ofSameValue(replacement, ImmutableList.of()).attach(input);
   }
 
+  public static FillNull fillNull(
+      UnresolvedPlan input, UnresolvedExpression replacement, boolean useValueSyntax) {
+    return FillNull.ofSameValue(replacement, ImmutableList.of(), useValueSyntax).attach(input);
+  }
+
   public static FillNull fillNull(
       UnresolvedPlan input, UnresolvedExpression replacement, Field... fields) {
     return FillNull.ofSameValue(replacement, ImmutableList.copyOf(fields)).attach(input);
   }
 
+  public static FillNull fillNull(
+      UnresolvedPlan input,
+      UnresolvedExpression replacement,
+      boolean useValueSyntax,
+      Field... fields) {
+    return FillNull.ofSameValue(replacement, ImmutableList.copyOf(fields), useValueSyntax)
+        .attach(input);
+  }
+
   public static FillNull fillNull(
       UnresolvedPlan input, List<Pair<Field, UnresolvedExpression>> fieldAndReplacements) {
     ImmutableList.Builder<Pair<Field, UnresolvedExpression>> replacementsBuilder =

core/src/main/java/org/opensearch/sql/ast/tree/FillNull.java

@@ -24,13 +24,18 @@
 public class FillNull extends UnresolvedPlan {
 
   public static FillNull ofVariousValue(List<Pair<Field, UnresolvedExpression>> replacements) {
-    return new FillNull(replacements);
+    return new FillNull(replacements, false);
   }
 
   public static FillNull ofSameValue(UnresolvedExpression replacement, List<Field> fieldList) {
+    return ofSameValue(replacement, fieldList, false);
+  }
+
+  public static FillNull ofSameValue(
+      UnresolvedExpression replacement, List<Field> fieldList, boolean useValueSyntax) {
     List<Pair<Field, UnresolvedExpression>> replacementPairs =
         fieldList.stream().map(f -> Pair.of(f, replacement)).toList();
-    FillNull instance = new FillNull(replacementPairs);
+    FillNull instance = new FillNull(replacementPairs, useValueSyntax);
     if (replacementPairs.isEmpty()) {
       // no field specified, the replacement value will be applied to all fields.
       instance.replacementForAll = Optional.of(replacement);
@@ -42,8 +47,14 @@ public static FillNull ofSameValue(UnresolvedExpression replacement, List<Field>
 
   private final List<Pair<Field, UnresolvedExpression>> replacementPairs;
 
-  FillNull(List<Pair<Field, UnresolvedExpression>> replacementPairs) {
+  // Track if value= syntax was used (added in 3.4). Only needed to distinguish from with...in
+  // since both apply same value to all fields. using syntax is detected by checking if all
+  // replacement values are the same.
+  private final boolean useValueSyntax;
+
+  FillNull(List<Pair<Field, UnresolvedExpression>> replacementPairs, boolean useValueSyntax) {
     this.replacementPairs = replacementPairs;
+    this.useValueSyntax = useValueSyntax;
   }
 
   private UnresolvedPlan child;

core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java

@@ -49,6 +49,7 @@
 import org.apache.calcite.rel.logical.LogicalAggregate;
 import org.apache.calcite.rel.logical.LogicalValues;
 import org.apache.calcite.rel.type.RelDataType;
+import org.apache.calcite.rel.type.RelDataTypeFamily;
 import org.apache.calcite.rel.type.RelDataTypeField;
 import org.apache.calcite.rex.RexCall;
 import org.apache.calcite.rex.RexCorrelVariable;
@@ -1493,6 +1494,30 @@ public RelNode visitWindow(Window node, CalcitePlanContext context) {
     return context.relBuilder.peek();
   }
 
+  /**
+   * Validates type compatibility between replacement value and field for fillnull operation. Throws
+   * SemanticCheckException if types are incompatible.
+   */
+  private void validateFillNullTypeCompatibility(
+      RexNode replacement, RexNode fieldRef, String fieldName) {
+    RelDataTypeFamily replacementFamily = replacement.getType().getFamily();
+    RelDataTypeFamily fieldFamily = fieldRef.getType().getFamily();
+
+    // Check if the replacement type is compatible with the field type
+    // Allow NULL type family as it's compatible with any type
+    if (fieldFamily != replacementFamily
+        && fieldFamily != SqlTypeFamily.NULL
+        && replacementFamily != SqlTypeFamily.NULL) {
+      throw new SemanticCheckException(
+          String.format(
+              "fillnull failed: replacement value type %s is not compatible with field '%s' "
+                  + "(type: %s). The replacement value type must match the field type.",
+              replacement.getType().getSqlTypeName(),
+              fieldName,
+              fieldRef.getType().getSqlTypeName()));
+    }
+  }
+
   @Override
   public RelNode visitFillNull(FillNull node, CalcitePlanContext context) {
     visitChildren(node, context);
@@ -1501,6 +1526,19 @@ public RelNode visitFillNull(FillNull node, CalcitePlanContext context) {
             .size()) {
       throw new IllegalArgumentException("The field list cannot be duplicated in fillnull");
     }
+
+    // Validate type compatibility when replacementForAll is present
+    if (node.getReplacementForAll().isPresent()) {
+      List<RelDataTypeField> fieldsList = context.relBuilder.peek().getRowType().getFieldList();
+      RexNode replacement = rexVisitor.analyze(node.getReplacementForAll().get(), context);
+
+      // Validate all fields are compatible with the replacement value
+      for (RelDataTypeField field : fieldsList) {
+        RexNode fieldRef = context.rexBuilder.makeInputRef(field.getType(), field.getIndex());
+        validateFillNullTypeCompatibility(replacement, fieldRef, field.getName());
+      }
+    }
+
     List<RexNode> projects = new ArrayList<>();
     List<RelDataTypeField> fieldsList = context.relBuilder.peek().getRowType().getFieldList();
     for (RelDataTypeField field : fieldsList) {
@@ -1509,6 +1547,8 @@ public RelNode visitFillNull(FillNull node, CalcitePlanContext context) {
       for (Pair<Field, UnresolvedExpression> pair : node.getReplacementPairs()) {
         if (field.getName().equalsIgnoreCase(pair.getLeft().getField().toString())) {
           RexNode replacement = rexVisitor.analyze(pair.getRight(), context);
+          // Validate type compatibility before COALESCE
+          validateFillNullTypeCompatibility(replacement, fieldRef, field.getName());
           RexNode coalesce = context.rexBuilder.coalesce(fieldRef, replacement);
           RexNode coalesceWithAlias = context.relBuilder.alias(coalesce, field.getName());
           projects.add(coalesceWithAlias);

docs/user/ppl/cmd/fillnull.rst

@@ -16,15 +16,33 @@ Using ``fillnull`` command to fill null with provided value in one or more field
 
 Syntax
 ============
+
 fillnull with <replacement> [in <field-list>]
 
 fillnull using <field> = <replacement> [, <field> = <replacement>]
 
-* replacement: mandatory. The value used to replace `null`s.
-* field-list: optional. The comma-delimited field list. The `null` values in the field will be replaced with the values from the replacement. From 3.1.0, when ``plugins.calcite.enabled`` is true, if no field specified, the replacement is applied to all fields.
+fillnull value=<replacement> [<field-list>]
+
+
+Parameters
+============
+
+* replacement: Mandatory. The value used to replace `null`s.
+
+* field-list: Optional. Comma-delimited (when using ``with`` or ``using``) or space-delimited (when using ``value=``) list of fields. The `null` values in the field will be replaced with the values from the replacement. **Default:** If no field specified, the replacement is applied to all fields.
+
+**Syntax Variations:**
+
+* ``with <replacement> in <field-list>`` - Apply same value to specified fields
+* ``using <field>=<replacement>, ...`` - Apply different values to different fields
+* ``value=<replacement> [<field-list>]`` - Alternative syntax with optional space-delimited field list
+
+
+Examples
+============
 
-Example 1: replace null values with a specified value on one field
-==================================================================
+Example 1: Replace null values with a specified value on one field
+-------------------------------------------------------------------
 
 PPL query::
 
@@ -39,8 +57,8 @@ PPL query::
     | [email protected]   | null     |
     +-----------------------+----------+
 
-Example 2: replace null values with a specified value on multiple fields
-========================================================================
+Example 2: Replace null values with a specified value on multiple fields
+-------------------------------------------------------------------------
 
 PPL query::
 
@@ -55,10 +73,8 @@ PPL query::
     | [email protected]   | <not found> |
     +-----------------------+-------------+
 
-Example 3: replace null values with a specified value on all fields
-===================================================================
-
-This example only works when Calcite enabled.
+Example 3: Replace null values with a specified value on all fields
+--------------------------------------------------------------------
 
 PPL query::
 
@@ -73,8 +89,8 @@ PPL query::
     | [email protected]   | <not found> |
     +-----------------------+-------------+
 
-Example 4: replace null values with multiple specified values on multiple fields
-================================================================================
+Example 4: Replace null values with multiple specified values on multiple fields
+---------------------------------------------------------------------------------
 
 PPL query::
 
@@ -90,7 +106,51 @@ PPL query::
     +-----------------------+---------------+
 
 
+Example 5: Replace null with specified value on specific fields (value= syntax)
+--------------------------------------------------------------------------------
+
+PPL query::
+
+    os> source=accounts | fields email, employer | fillnull value="<not found>" email employer;
+    fetched rows / total rows = 4/4
+    +-----------------------+-------------+
+    | email                 | employer    |
+    |-----------------------+-------------|
+    | [email protected]  | Pyrami      |
+    | [email protected] | Netagy      |
+    | <not found>           | Quility     |
+    | [email protected]   | <not found> |
+    +-----------------------+-------------+
+
+Example 6: Replace null with specified value on all fields (value= syntax)
+---------------------------------------------------------------------------
+
+When no field list is specified, the replacement applies to all fields in the result.
+
+PPL query::
+
+    os> source=accounts | fields email, employer | fillnull value='<not found>';
+    fetched rows / total rows = 4/4
+    +-----------------------+-------------+
+    | email                 | employer    |
+    |-----------------------+-------------|
+    | [email protected]  | Pyrami      |
+    | [email protected] | Netagy      |
+    | <not found>           | Quility     |
+    | [email protected]   | <not found> |
+    +-----------------------+-------------+
+
 Limitations
-===========
+============
 * The ``fillnull`` command is not rewritten to OpenSearch DSL, it is only executed on the coordination node.
-* Before 3.1.0, at least one field is required.
+* When applying the same value to all fields without specifying field names, all fields must be the same type. For mixed types, use separate fillnull commands or explicitly specify fields.
+* The replacement value type must match ALL field types in the field list. When applying the same value to multiple fields, all fields must be the same type (all strings or all numeric).
+
+  **Example:**
+
+  .. code-block:: sql
+
+     # This FAILS - same value for mixed-type fields
+     source=accounts | fillnull value=0 firstname, age
+     # ERROR: fillnull failed: replacement value type INTEGER is not compatible with field 'firstname' (type: VARCHAR). The replacement value type must match the field type.
+

integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java

@@ -936,4 +936,14 @@ public void testExplainPushDownScriptsContainingUDT() throws IOException {
                     + " span(t, 1d)",
                 TEST_INDEX_BANK)));
   }
+
+  @Test
+  public void testFillNullValueSyntaxExplain() throws IOException {
+    String expected = loadExpectedPlan("explain_fillnull_value_syntax.yaml");
+    assertYamlEqualsJsonIgnoreId(
+        expected,
+        explainQueryToString(
+            String.format(
+                "source=%s | fields age, balance | fillnull value=0", TEST_INDEX_ACCOUNT)));
+  }
 }