Always lower a bucket function to a span

Review feedback: a histogram expression should end up as an OpenSearch
histogram aggregate, so generating date_format or timestampadd was surprising.
It is, and they are gone -- `format` and `time_zone` now defer to the legacy
engine along with alias, min_doc_count and order.

The legacy engine implements all five natively (AggMaker builds them straight
onto the date_histogram aggregation), and for time_zone it does so better:
`dateHistogram.timeZone(ZoneOffset.of(value))` shifts bucket boundaries
properly, where this code was adding a fixed number of seconds and would have
been wrong across a daylight-saving change. Handing those queries back means
they are answered by the implementation that already had them right.

What is left always produces a Span, which is what the earlier comment about
lowering to existing AST primitives described. `missing` still wraps the field
in `ifnull`, since substituting a value has to happen before bucketing.

Verified on a live cluster: the plain and `missing` forms answer from V2
(12/24/17/19 and 19/20/20/13), while `format`, `time_zone` and `alias` reach
legacy and answer correctly -- time_zone returning 5/18/30/19, the shifted
boundaries. 983 default-route tests pass, and `:sql:build` is green including
the coverage gate.

Signed-off-by: Jialiang Liang <ryanleeang@gmail.com>

Author: RyanL1997

integ-test/src/test/java/org/opensearch/sql/sql/DateHistogramBucketFunctionIT.java

@@ -186,8 +186,9 @@ public void positionalCallReturnsHourlyBuckets() throws IOException {
   }
 
   /**
-   * `alias` has no lowering here but the legacy engine implements it, so the query still has to
-   * answer. CsvFormatResponseIT.dateHistogramTest has asserted this shape for years.
+   * The legacy engine implements alias, format, time_zone, min_doc_count and order through the
+   * native date_histogram aggregation; this lowering has no equivalent, so those queries still have
+   * to reach it. CsvFormatResponseIT.dateHistogramTest has asserted this shape for years.
    */
   @Test
   @RequiresCapability(LEGACY_ENGINE_FALLBACK)

sql/src/main/java/org/opensearch/sql/sql/parser/AstExpressionBuilder.java

@@ -72,7 +72,6 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import java.time.ZoneOffset;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
@@ -193,23 +192,18 @@ public UnresolvedExpression visitBucketFunctionCall(BucketFunctionCallContext ct
     UnresolvedExpression field = requireArg(args, "field", functionName);
     UnresolvedExpression missing = args.remove("missing");
     Literal interval = intervalOf(args, functionName);
-    Literal format = stringArg(args, "format");
-    Literal timeZone = stringArg(args, "time_zone");
 
-    // Anything left is a parameter with no lowering here. Some of them -- alias, min_doc_count,
-    // order -- are implemented by the legacy engine, so decline in the one way RestSQLQueryAction
-    // falls back on rather than failing the request outright.
+    // Anything left is a parameter this lowering has no equivalent for. The legacy engine
+    // implements all of them -- alias, format, time_zone, min_doc_count, order -- through the
+    // native date_histogram aggregation, so decline in the one way RestSQLQueryAction falls back
+    // on rather than failing a query it can answer.
     if (!args.isEmpty()) {
       throw new SyntaxCheckException(
           functionName + " does not accept parameter: " + String.join(", ", args.keySet()));
     }
 
-    UnresolvedExpression bucketed = substituteMissing(normalizeField(field), missing);
-    if (timeZone != null) {
-      bucketed = shiftByTimeZone(bucketed, timeZone);
-    }
-    Span span = AstDSL.spanFromSpanLengthLiteral(bucketed, interval);
-    return format == null ? span : new Function("date_format", List.of(span, format));
+    return AstDSL.spanFromSpanLengthLiteral(
+        substituteMissing(normalizeField(field), missing), interval);
   }
 
   private static UnresolvedExpression requireArg(
@@ -242,18 +236,6 @@ private static Literal intervalOf(Map<String, UnresolvedExpression> args, String
     return supplied.get(0);
   }
 
-  private static Literal stringArg(Map<String, UnresolvedExpression> args, String name) {
-    UnresolvedExpression value = args.remove(name);
-    if (value == null) {
-      return null;
-    }
-    if (!(value instanceof Literal literal) || literal.getType() != DataType.STRING) {
-      throw new SemanticCheckException(
-          name + " must be a string literal (e.g. '1d', '15m'); got " + value);
-    }
-    return literal;
-  }
-
   private static Literal stringOrNumericArg(Map<String, UnresolvedExpression> args, String name) {
     UnresolvedExpression value = args.remove(name);
     if (value == null) {
@@ -283,24 +265,6 @@ private static UnresolvedExpression substituteMissing(
     return missing == null ? field : new Function("ifnull", List.of(field, missing));
   }
 
-  /**
-   * Shifts the field by a {@link ZoneOffset} before bucketing. Validated here so an invalid offset
-   * is reported rather than surfacing as an arithmetic failure at execution.
-   */
-  private static UnresolvedExpression shiftByTimeZone(
-      UnresolvedExpression field, Literal timeZone) {
-    String offset = timeZone.getValue().toString();
-    int seconds;
-    try {
-      seconds = ZoneOffset.of(offset).getTotalSeconds();
-    } catch (RuntimeException e) {
-      throw new SemanticCheckException(
-          "time_zone must be a valid offset like '+05:30' or 'Z'; got '" + offset + "'");
-    }
-    return new Function(
-        "timestampadd", List.of(AstDSL.stringLiteral("SECOND"), AstDSL.intLiteral(seconds), field));
-  }
-
   @Override
   public UnresolvedExpression visitGetFormatFunctionCall(GetFormatFunctionCallContext ctx) {
     return new Function(

sql/src/test/java/org/opensearch/sql/sql/parser/AstExpressionBuilderTest.java

@@ -910,27 +910,6 @@ public void canBuildDateHistogramWithMissing() {
         buildExprAst("date_histogram('field'=ts, 'interval'='1h', 'missing'='1970-01-01')"));
   }
 
-  @Test
-  public void canBuildDateHistogramWithTimeZoneShift() {
-    assertEquals(
-        new Span(
-            function(
-                "timestampadd", stringLiteral("SECOND"), intLiteral(19800), qualifiedName("ts")),
-            intLiteral(1),
-            SpanUnit.H),
-        buildExprAst("date_histogram('field'=ts, 'interval'='1h', 'time_zone'='+05:30')"));
-  }
-
-  @Test
-  public void canBuildDateHistogramWithFormat() {
-    assertEquals(
-        function(
-            "date_format",
-            new Span(qualifiedName("ts"), intLiteral(1), SpanUnit.D),
-            stringLiteral("yyyy-MM-dd")),
-        buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'format'='yyyy-MM-dd')"));
-  }
-
   /**
    * A parameter with no lowering here has to raise SyntaxCheckException -- the one type
    * RestSQLQueryAction falls back on -- because the legacy engine implements alias, min_doc_count
@@ -944,6 +923,12 @@ public void unsupportedBucketParameterDefersToLegacyEngine() {
     assertThrows(
         SyntaxCheckException.class,
         () -> buildExprAst("histogram('field'=age, 'interval'=10, 'min_doc_count'=1)"));
+    assertThrows(
+        SyntaxCheckException.class,
+        () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'format'='yyyy-MM-dd')"));
+    assertThrows(
+        SyntaxCheckException.class,
+        () -> buildExprAst("date_histogram('field'=ts, 'interval'='1h', 'time_zone'='+05:30')"));
   }
 
   /** A bad argument inside a shape we own must not fall back, so the caller sees this message. */
@@ -955,15 +940,6 @@ public void badBucketArgumentIsReportedRatherThanDeferred() {
     assertThrows(
         SemanticCheckException.class,
         () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'fixed_interval'='2d')"));
-    assertThrows(
-        SemanticCheckException.class,
-        () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'time_zone'='nope')"));
-    assertThrows(
-        SemanticCheckException.class,
-        () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'format'=7)"));
-    assertThrows(
-        SemanticCheckException.class,
-        () -> buildExprAst("date_histogram('field'=ts, 'interval'='1d', 'format'=other)"));
     assertThrows(
         SemanticCheckException.class,
         () -> buildExprAst("date_histogram('field'=ts, 'interval'=ts)"));