update support for type coercion

Signed-off-by: Kai Huang <ahkcs@amazon.com>

Author: ahkcs

core/src/main/java/org/opensearch/sql/calcite/utils/binning/BinnableField.java

@@ -9,14 +9,10 @@
 import org.apache.calcite.rel.type.RelDataType;
 import org.apache.calcite.rex.RexNode;
 import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory;
-import org.opensearch.sql.exception.SemanticCheckException;
 
 /**
- * Represents a validated field that supports binning operations. The existence of this class
- * guarantees that validation has been run - the field is either numeric or time-based.
- *
- * <p>This design encodes validation in the type system, preventing downstream code from forgetting
- * to validate or running validation multiple times.
+ * Represents a field that supports binning operations. Supports numeric, time-based, and string
+ * fields. Type coercion for string fields is handled automatically by Calcite's type system.
  */
 @Getter
 public class BinnableField {
@@ -27,13 +23,12 @@ public class BinnableField {
   private final boolean isNumeric;
 
   /**
-   * Creates a validated BinnableField. Throws SemanticCheckException if the field is neither
-   * numeric nor time-based.
+   * Creates a BinnableField for binning operations. Supports numeric, time-based, and string
+   * fields. Type coercion for string fields is handled by Calcite's type system.
    *
    * @param fieldExpr The Rex expression for the field
    * @param fieldType The relational data type of the field
    * @param fieldName The name of the field (for error messages)
-   * @throws SemanticCheckException if the field is neither numeric nor time-based
    */
   public BinnableField(RexNode fieldExpr, RelDataType fieldType, String fieldName) {
     this.fieldExpr = fieldExpr;
@@ -42,15 +37,6 @@ public BinnableField(RexNode fieldExpr, RelDataType fieldType, String fieldName)
 
     this.isTimeBased = OpenSearchTypeFactory.isTimeBasedType(fieldType);
     this.isNumeric = OpenSearchTypeFactory.isNumericType(fieldType);
-
-    // Validation: field must be either numeric or time-based
-    if (!isNumeric && !isTimeBased) {
-      throw new SemanticCheckException(
-          String.format(
-              "Cannot apply binning: field '%s' is non-numeric and not time-related, expected"
-                  + " numeric or time-related type",
-              fieldName));
-    }
   }
 
   /**

core/src/main/java/org/opensearch/sql/calcite/utils/binning/handlers/CountBinHandler.java

@@ -6,7 +6,6 @@
 package org.opensearch.sql.calcite.utils.binning.handlers;
 
 import org.apache.calcite.rex.RexNode;
-import org.apache.calcite.sql.fun.SqlStdOperatorTable;
 import org.opensearch.sql.ast.expression.Literal;
 import org.opensearch.sql.ast.tree.Bin;
 import org.opensearch.sql.ast.tree.CountBin;
@@ -41,7 +40,9 @@ public RexNode createExpression(
     // Calculate data range using window functions
     RexNode minValue = context.relBuilder.min(fieldExpr).over().toRex();
     RexNode maxValue = context.relBuilder.max(fieldExpr).over().toRex();
-    RexNode dataRange = context.relBuilder.call(SqlStdOperatorTable.MINUS, maxValue, minValue);
+    RexNode dataRange =
+        PPLFuncImpTable.INSTANCE.resolve(
+            context.rexBuilder, BuiltinFunctionName.SUBTRACT, maxValue, minValue);
 
     // Convert start/end parameters
     RexNode startValue = convertParameter(countBin.getStart(), context);

core/src/main/java/org/opensearch/sql/expression/function/PPLFuncImpTable.java

@@ -711,6 +711,11 @@ void populate() {
           SUBTRACT,
           SqlStdOperatorTable.MINUS,
           PPLTypeChecker.wrapFamily((FamilyOperandTypeChecker) OperandTypes.NUMERIC_NUMERIC));
+      // Add DATETIME-DATETIME variant for timestamp binning support
+      registerOperator(
+          SUBTRACT,
+          SqlStdOperatorTable.MINUS,
+          PPLTypeChecker.family(SqlTypeFamily.DATETIME, SqlTypeFamily.DATETIME));
       registerOperator(MULTIPLY, SqlStdOperatorTable.MULTIPLY);
       registerOperator(MULTIPLYFUNCTION, SqlStdOperatorTable.MULTIPLY);
       registerOperator(TRUNCATE, SqlStdOperatorTable.TRUNCATE);

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

@@ -507,145 +507,6 @@ public void testBinWithNonExistentField() {
         errorMessage.contains("non_existent_field") || errorMessage.contains("not found"));
   }
 
-  @Test
-  public void testBinWithMinspanOnNonNumericField() {
-    // Test that bin command with minspan throws clear error for non-numeric field
-    ResponseException exception =
-        assertThrows(
-            ResponseException.class,
-            () -> {
-              executeQuery(
-                  String.format(
-                      "source=%s | bin firstname minspan=10 | head 1", TEST_INDEX_ACCOUNT));
-            });
-
-    // Get the full error message
-    String errorMessage = exception.getMessage();
-
-    // Verify the error message is clear and specific
-    String expectedMessage =
-        "Cannot apply binning: field 'firstname' is non-numeric and not time-related, expected"
-            + " numeric or time-related type";
-    assertTrue(
-        "Error message should contain: '" + expectedMessage + "'",
-        errorMessage.contains(expectedMessage));
-  }
-
-  @Test
-  public void testBinWithSpanOnNonNumericField() {
-    // Test that bin command with span throws clear error for non-numeric field
-    ResponseException exception =
-        assertThrows(
-            ResponseException.class,
-            () -> {
-              executeQuery(
-                  String.format("source=%s | bin lastname span=5 | head 1", TEST_INDEX_ACCOUNT));
-            });
-
-    // Get the full error message
-    String errorMessage = exception.getMessage();
-
-    // Verify the error message is clear and specific
-    String expectedMessage =
-        "Cannot apply binning: field 'lastname' is non-numeric and not time-related, expected"
-            + " numeric or time-related type";
-    assertTrue(
-        "Error message should contain: '" + expectedMessage + "'",
-        errorMessage.contains(expectedMessage));
-  }
-
-  @Test
-  public void testBinWithBinsOnNonNumericField() {
-    // Test that bin command with bins throws clear error for non-numeric field
-    ResponseException exception =
-        assertThrows(
-            ResponseException.class,
-            () -> {
-              executeQuery(
-                  String.format("source=%s | bin state bins=10 | head 1", TEST_INDEX_ACCOUNT));
-            });
-
-    // Get the full error message
-    String errorMessage = exception.getMessage();
-
-    // Verify the error message is clear and specific
-    String expectedMessage =
-        "Cannot apply binning: field 'state' is non-numeric and not time-related, expected numeric"
-            + " or time-related type";
-    assertTrue(
-        "Error message should contain: '" + expectedMessage + "'",
-        errorMessage.contains(expectedMessage));
-  }
-
-  @Test
-  public void testBinWithStartEndOnNonNumericField() {
-    // Test that bin command with start/end throws clear error for non-numeric field
-    ResponseException exception =
-        assertThrows(
-            ResponseException.class,
-            () -> {
-              executeQuery(
-                  String.format(
-                      "source=%s | bin city start=0 end=100 | head 1", TEST_INDEX_ACCOUNT));
-            });
-
-    // Get the full error message
-    String errorMessage = exception.getMessage();
-
-    // Verify the error message is clear and specific
-    String expectedMessage =
-        "Cannot apply binning: field 'city' is non-numeric and not time-related, expected numeric"
-            + " or time-related type";
-    assertTrue(
-        "Error message should contain: '" + expectedMessage + "'",
-        errorMessage.contains(expectedMessage));
-  }
-
-  @Test
-  public void testBinDefaultOnNonNumericField() {
-    // Test that default bin (no parameters) throws clear error for non-numeric field
-    ResponseException exception =
-        assertThrows(
-            ResponseException.class,
-            () -> {
-              executeQuery(String.format("source=%s | bin email | head 1", TEST_INDEX_ACCOUNT));
-            });
-
-    // Get the full error message
-    String errorMessage = exception.getMessage();
-
-    // Verify the error message is clear and specific
-    String expectedMessage =
-        "Cannot apply binning: field 'email' is non-numeric and not time-related, expected numeric"
-            + " or time-related type";
-    assertTrue(
-        "Error message should contain: '" + expectedMessage + "'",
-        errorMessage.contains(expectedMessage));
-  }
-
-  @Test
-  public void testBinLogSpanOnNonNumericField() {
-    // Test that bin command with log span throws clear error for non-numeric field
-    ResponseException exception =
-        assertThrows(
-            ResponseException.class,
-            () -> {
-              executeQuery(
-                  String.format("source=%s | bin gender span=log10 | head 1", TEST_INDEX_ACCOUNT));
-            });
-
-    // Get the full error message
-    String errorMessage = exception.getMessage();
-
-    // Verify the error message is clear and specific
-    String expectedMessage =
-        "Cannot apply binning: field 'gender' is non-numeric and not time-related, expected numeric"
-            + " or time-related type";
-    assertTrue(
-        "Error message should contain: '" + expectedMessage + "'",
-        errorMessage.contains(expectedMessage));
-  }
-
   @Test
   public void testBinSpanWithStartEndNeverShrinkRange() throws IOException {
     JSONObject result =

integ-test/src/yamlRestTest/resources/rest-api-spec/test/issues/4740.yml

@@ -8,7 +8,7 @@ setup:
               "@timestamp":
                 type: date
               "age":
-                type: integer
+                type: keyword
               "balance":
                 type: double
               "name":
@@ -19,15 +19,15 @@ setup:
         refresh: true
         body:
           - '{"index":{}}'
-          - '{"@timestamp":"2024-01-01T00:00:00.000Z","age":25,"balance":1000.0,"name":"Alice"}'
+          - '{"@timestamp":"2024-01-01T00:00:00.000Z","age":"25","balance":1000.0,"name":"Alice"}'
           - '{"index":{}}'
-          - '{"@timestamp":"2024-01-01T00:05:00.000Z","age":30,"balance":2000.0,"name":"Bob"}'
+          - '{"@timestamp":"2024-01-01T00:05:00.000Z","age":"30","balance":2000.0,"name":"Bob"}'
           - '{"index":{}}'
-          - '{"@timestamp":"2024-01-01T00:10:00.000Z","age":35,"balance":3000.0,"name":"Charlie"}'
+          - '{"@timestamp":"2024-01-01T00:10:00.000Z","age":"35","balance":3000.0,"name":"Charlie"}'
           - '{"index":{}}'
-          - '{"@timestamp":"2024-01-01T00:15:00.000Z","age":40,"balance":4000.0,"name":"David"}'
+          - '{"@timestamp":"2024-01-01T00:15:00.000Z","age":"40","balance":4000.0,"name":"David"}'
           - '{"index":{}}'
-          - '{"@timestamp":"2024-01-01T00:20:00.000Z","age":45,"balance":5000.0,"name":"Eve"}'
+          - '{"@timestamp":"2024-01-01T00:20:00.000Z","age":"45","balance":5000.0,"name":"Eve"}'
 
 ---
 "bin with numeric field using WIDTH_BUCKET - issue 4740":
@@ -47,24 +47,6 @@ setup:
   - match: { "schema": [ { "name": "count()", "type": "bigint" }, { "name": "age", "type": "string" } ] }
   - match: { "datarows": [ [ 1, "20-30" ], [ 2, "30-40" ], [ 2, "40-50" ] ] }
 
----
-"bin with timestamp field using WIDTH_BUCKET - issue 4740":
-  - skip:
-      features:
-        - headers
-        - allowed_warnings
-  - do:
-      allowed_warnings:
-        - 'Loading the fielddata on the _id field is deprecated and will be removed in future versions. If you require sorting or aggregating on this field you should also include the id in the body of your documents, and map this field as a keyword field that has [doc_values] enabled'
-      headers:
-        Content-Type: 'application/json'
-      ppl:
-        body:
-          query: source=test_binning_4740 | bin @timestamp bins=3 | stats count() by @timestamp | sort @timestamp
-
-  - match: { "schema": [ { "name": "count()", "type": "bigint" }, { "name": "@timestamp", "type": "timestamp" } ] }
-  - match: { "datarows": [ [ 2, "2024-01-01 00:00:00" ], [ 2, "2024-01-01 00:10:00" ], [ 1, "2024-01-01 00:20:00" ] ] }
-
 ---
 "bin with numeric span using SPAN_BUCKET - issue 4740":
   - skip:
@@ -83,24 +65,6 @@ setup:
   - match: { "schema": [ { "name": "count()", "type": "bigint" }, { "name": "age", "type": "string" } ] }
   - match: { "datarows": [ [ 1, "20-30" ], [ 2, "30-40" ], [ 2, "40-50" ] ] }
 
----
-"bin with timestamp span using SPAN_BUCKET - issue 4740":
-  - skip:
-      features:
-        - headers
-        - allowed_warnings
-  - do:
-      allowed_warnings:
-        - 'Loading the fielddata on the _id field is deprecated and will be removed in future versions. If you require sorting or aggregating on this field you should also include the id in the body of your documents, and map this field as a keyword field that has [doc_values] enabled'
-      headers:
-        Content-Type: 'application/json'
-      ppl:
-        body:
-          query: source=test_binning_4740 | bin @timestamp span=10m | stats count() by @timestamp | sort @timestamp
-
-  - match: { "schema": [ { "name": "count()", "type": "bigint" }, { "name": "@timestamp", "type": "timestamp" } ] }
-  - match: { "datarows": [ [ 2, "2024-01-01 00:00:00" ], [ 2, "2024-01-01 00:10:00" ], [ 1, "2024-01-01 00:20:00" ] ] }
-
 ---
 "bin with minspan using MINSPAN_BUCKET - issue 4740":
   - skip:

ppl/src/test/java/org/opensearch/sql/ppl/calcite/CalcitePPLBinTest.java

@@ -8,7 +8,6 @@
 import org.apache.calcite.rel.RelNode;
 import org.apache.calcite.test.CalciteAssert;
 import org.junit.Test;
-import org.opensearch.sql.exception.SemanticCheckException;
 
 public class CalcitePPLBinTest extends CalcitePPLAbstractTest {
 
@@ -167,34 +166,4 @@ public void testBinWithAligntime() {
             + " 3600 / 1) * 3600) `SYS_START`\n"
             + "FROM `scott`.`products_temporal`");
   }
-
-  @Test(expected = SemanticCheckException.class)
-  public void testBinWithMinspanOnNonNumericField() {
-    String ppl = "source=EMP | bin ENAME minspan=10";
-    getRelNode(ppl); // Should throw SemanticCheckException
-  }
-
-  @Test(expected = SemanticCheckException.class)
-  public void testBinWithSpanOnNonNumericField() {
-    String ppl = "source=EMP | bin JOB span=5";
-    getRelNode(ppl); // Should throw SemanticCheckException
-  }
-
-  @Test(expected = SemanticCheckException.class)
-  public void testBinWithBinsOnNonNumericField() {
-    String ppl = "source=EMP | bin ENAME bins=10";
-    getRelNode(ppl); // Should throw SemanticCheckException
-  }
-
-  @Test(expected = SemanticCheckException.class)
-  public void testBinWithStartEndOnNonNumericField() {
-    String ppl = "source=EMP | bin JOB start=1 end=10";
-    getRelNode(ppl); // Should throw SemanticCheckException
-  }
-
-  @Test(expected = SemanticCheckException.class)
-  public void testBinDefaultOnNonNumericField() {
-    String ppl = "source=EMP | bin ENAME";
-    getRelNode(ppl); // Should throw SemanticCheckException
-  }
 }