ES|QL: Address PR review comments

- Add more tests (in TS mode and in Standard mode)
- Add TestClassSignatureTypes to fix docs generation when there are
more than on constructors and not all parameters are exposed publicly.
- Improve TBucket types resolution
- Add SubstituteSurrogateExpressions before TranslateTimeSeriesAggregate
 to substitute TBucket by Bucket before it is used in
 TimeSeriesAggregate

Closes #131068

Author: leontyevdv

docs/reference/query-languages/esql/_snippets/functions/types/tbucket.md

@@ -0,0 +1,16 @@
+{
+    "properties": {
+        "@timestamp": {
+            "type": "boolean"
+        },
+        "client_ip": {
+            "type": "ip"
+        },
+        "event_duration": {
+            "type": "long"
+        },
+        "message": {
+            "type": "keyword"
+        }
+    }
+}

docs/reference/query-languages/esql/kibana/definition/functions/tbucket.json

@@ -89,6 +89,12 @@ TS k8s
 // end::rate[]
 | SORT time_bucket DESC | LIMIT 2;
 
+oneRateWithTBucket
+required_capability: metrics_command
+TS k8s
+| STATS max(rate(network.total_bytes_in)) BY time_bucket = tbucket(5minute)
+| SORT time_bucket DESC | LIMIT 2;
+
 // tag::rate-result[]
 max(rate(network.total_bytes_in)): double | time_bucket:date
 6.980660660660663                         | 2024-05-10T00:20:00.000Z
@@ -106,6 +112,15 @@ max(rate(network.total_bytes_in)):double | sum(rate(network.total_bytes_in)):dou
 14.97039381153305                        | 63.00652190262822                        | 2024-05-10T00:10:00.000Z
 ;
 
+twoRatesWithTBucket
+required_capability: metrics_command
+TS k8s | STATS max(rate(network.total_bytes_in)), sum(rate(network.total_bytes_in)) BY time_bucket = tbucket(5minute) | SORT time_bucket DESC | LIMIT 3;
+
+max(rate(network.total_bytes_in)):double | sum(rate(network.total_bytes_in)):double | time_bucket:datetime    
+6.980660660660663                        | 23.959973363184154                       | 2024-05-10T00:20:00.000Z
+23.702205882352942                       | 94.9517511187984                         | 2024-05-10T00:15:00.000Z
+14.97039381153305                        | 63.00652190262822                        | 2024-05-10T00:10:00.000Z
+;
 
 oneRateWithBucketAndCluster
 required_capability: metrics_command

x-pack/plugin/esql-core/src/test/resources/mapping-odd-timestamp.json

@@ -25,6 +25,7 @@
 import java.util.List;
 
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.DEFAULT;
+import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.ParamOrdinal.SECOND;
 import static org.elasticsearch.xpack.esql.core.expression.TypeResolutions.isType;
 
 /**
@@ -97,7 +98,9 @@ protected TypeResolution resolveType() {
         if (childrenResolved() == false) {
             return new TypeResolution("Unresolved children");
         }
-        return isType(buckets, DataType::isTemporalAmount, sourceText(), DEFAULT, "date_period", "time_duration");
+        return isType(buckets, DataType::isTemporalAmount, sourceText(), DEFAULT, "date_period", "time_duration").and(
+            isType(timestamp, dt -> dt == DataType.DATETIME || dt == DataType.DATE_NANOS, sourceText(), SECOND, "date_nanos or datetime")
+        );
     }
 
     @Override

x-pack/plugin/esql/qa/testFixtures/src/main/resources/k8s-timeseries.csv-spec

@@ -142,6 +142,7 @@ protected static Batch<LogicalPlan> substitutions() {
             new ReplaceAggregateAggExpressionWithEval(),
             // lastly replace surrogate functions
             new SubstituteSurrogateAggregations(),
+            new SubstituteSurrogateExpressions(),
             // translate metric aggregates after surrogate substitution and replace nested expressions with eval (again)
             new TranslateTimeSeriesAggregate(),
             new PruneUnusedIndexMode(),

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/grouping/TBucket.java

@@ -4117,7 +4117,7 @@ public void testGroupingOverridesInInlinestats() {
             """, "Found 1 problem\n" + "line 2:49: Unknown column [x]", "mapping-default.json");
     }
 
-    public void testTBucketWithIntervalInStringInBothAggregationAndGrouping() {
+    public void testTBucketWithDatePeriodInBothAggregationAndGrouping() {
         LogicalPlan plan = analyze("""
             FROM sample_data
             | STATS min = MIN(@timestamp), max = MAX(@timestamp) BY bucket = TBUCKET(1 week)

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/LogicalPlanOptimizer.java

@@ -76,6 +76,8 @@ public class VerifierTests extends ESTestCase {
     private static final EsqlParser parser = new EsqlParser();
     private final Analyzer defaultAnalyzer = AnalyzerTestUtils.expandedDefaultAnalyzer();
     private final Analyzer fullTextAnalyzer = AnalyzerTestUtils.analyzer(loadMapping("mapping-full_text_search.json", "test"));
+    private final Analyzer sampleDataAnalyzer = AnalyzerTestUtils.analyzer(loadMapping("mapping-sample_data.json", "test"));
+    private final Analyzer oddSampleDataAnalyzer = AnalyzerTestUtils.analyzer(loadMapping("mapping-odd-timestamp.json", "test"));
     private final Analyzer tsdb = AnalyzerTestUtils.analyzer(AnalyzerTestUtils.tsdbIndexResolution());
 
     private final List<String> TIME_DURATIONS = List.of("millisecond", "second", "minute", "hour");
@@ -2501,6 +2503,38 @@ public void testVectorSimilarityFunctionsNullArgs() throws Exception {
         }
     }
 
+    public void testInvalidTBucketCalls() {
+        assertThat(error("from test | stats max(emp_no) by tbucket(1 hour)"), equalTo("1:34: Unknown column [@timestamp]"));
+        assertThat(
+            error("from test | stats max(event_duration) by tbucket()", sampleDataAnalyzer, ParsingException.class),
+            equalTo("1:42: error building [tbucket]: expects exactly one argument")
+        );
+        assertThat(
+            error("from test | stats max(event_duration) by tbucket(\"@tbucket\", 1 hour)", sampleDataAnalyzer, ParsingException.class),
+            equalTo("1:42: error building [tbucket]: expects exactly one argument")
+        );
+        assertThat(
+            error("from test | stats max(event_duration) by tbucket(1 hr)", sampleDataAnalyzer, ParsingException.class),
+            equalTo("1:50: Unexpected temporal unit: 'hr'")
+        );
+        assertThat(
+            error("from test | stats max(event_duration) by tbucket(\"1\")", sampleDataAnalyzer),
+            equalTo("1:42: argument of [tbucket(\"1\")] must be [date_period or time_duration], found value [\"1\"] type [keyword]")
+        );
+        assertThat(
+            error("from test | stats max(event_duration) by tbucket(\"1 hour\")", oddSampleDataAnalyzer),
+            equalTo(
+                "1:42: second argument of [tbucket(\"1 hour\")] must be [date_nanos or datetime], found value [@timestamp] type [boolean]"
+            )
+        );
+        for (String interval : List.of("1 minu", "1 dy", "1.5 minutes", "0.5 days", "minutes 1", "day 5")) {
+            assertThat(
+                error("from test | stats max(event_duration) by tbucket(\"" + interval + "\")", sampleDataAnalyzer),
+                containsString("1:50: Cannot convert string [" + interval + "] to [DATE_PERIOD or TIME_DURATION]")
+            );
+        }
+    }
+
     private void checkVectorSimilarityFunctionsNullArgs(String functionInvocation, String argOrdinal) throws Exception {
         assertThat(
             error("from test | eval similarity = " + functionInvocation, fullTextAnalyzer),

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java

@@ -66,7 +66,6 @@
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
@@ -758,13 +757,13 @@ public static void testFunctionInfo() {
         for (int i = 0; i < args.size(); i++) {
             typesFromSignature.add(new HashSet<>());
         }
-        for (Map.Entry<List<DataType>, DataType> entry : signatures(testClass).entrySet()) {
-            List<DataType> types = entry.getKey();
+        for (TestClassSignatureTypes entry : signatures(testClass)) {
+            List<DataType> types = entry.argTypes();
             for (int i = 0; i < args.size() && i < types.size(); i++) {
                 typesFromSignature.get(i).add(types.get(i).esNameIfPossible());
             }
-            if (DataType.UNDER_CONSTRUCTION.containsKey(entry.getValue()) == false) {
-                returnFromSignature.add(entry.getValue().esNameIfPossible());
+            if (DataType.UNDER_CONSTRUCTION.containsKey(entry.returnType()) == false) {
+                returnFromSignature.add(entry.returnType().esNameIfPossible());
             }
         }
 
@@ -838,7 +837,8 @@ public static void testFunctionLicenseChecks() throws Exception {
         TestCheckLicense checkLicense = new TestCheckLicense();
 
         // Go through all signatures and assert that the license is as expected
-        signatures(testClass).forEach((signature, returnType) -> {
+        signatures(testClass).forEach((signatureItem) -> {
+            List<DataType> signature = signatureItem.argTypes();
             try {
                 License.OperationMode license = licenseChecker.invoke(signature);
                 assertNotNull("License should not be null", license);
@@ -933,14 +933,14 @@ protected final void assertTypeResolutionFailure(Expression expression) {
     /**
      * Unique signatures in this test’s parameters.
      */
-    private static Map<List<DataType>, DataType> signatures;
+    private static Set<TestClassSignatureTypes> signatures;
 
-    public static Map<List<DataType>, DataType> signatures(Class<?> testClass) {
+    public static Set<TestClassSignatureTypes> signatures(Class<?> testClass) {
         if (signatures != null && classGeneratingSignatures == testClass) {
             return signatures;
         }
         classGeneratingSignatures = testClass;
-        signatures = new HashMap<>();
+        signatures = new HashSet<>();
         Set<Method> paramsFactories = new ClassModel(testClass).getAnnotatedLeafMethods(ParametersFactory.class).keySet();
         assertThat(paramsFactories, hasSize(1));
         Method paramsFactory = paramsFactories.iterator().next();
@@ -960,7 +960,7 @@ public static Map<List<DataType>, DataType> signatures(Class<?> testClass) {
                 continue;
             }
             List<DataType> types = tc.getData().stream().map(TestCaseSupplier.TypedData::type).toList();
-            signatures.putIfAbsent(signatureTypes(testClass, types), tc.expectedType());
+            signatures.add(new TestClassSignatureTypes(signatureTypes(testClass, types), tc.expectedType()));
         }
         return signatures;
     }