A random-random test for time-series data #132556

pabloem · 2025-08-07T23:58:36Z

Follow up items after this PR:

Test rate function and counters in general
Randomize window size
Increase test size
Hit some more corner cases (e.g. zero out some parameters)

elasticsearchmachine · 2025-08-11T18:08:31Z

Pinging @elastic/es-storage-engine (Team:StorageEngine)

not-napoleon

I'd like to get some confirmation from @kkrik-es that this is doing what he wants, but I think it's pretty good. I left some feedback, none of which is critical but I'd like to get it addressed.

not-napoleon · 2025-08-11T18:21:48Z

test/framework/src/main/java/org/elasticsearch/datageneration/FieldType.java

@@ -78,6 +79,7 @@ public FieldDataGenerator generator(String fieldName, DataSource dataSource) {
            case IP -> new IpFieldDataGenerator(dataSource);
            case CONSTANT_KEYWORD -> new ConstantKeywordFieldDataGenerator();
            case WILDCARD -> new WildcardFieldDataGenerator(dataSource);
+            default -> null;


I would prefer not to have a default in this switch. When someone adds a new FieldType here, I want the compiler to tell them they must also update this switch, but the default will hide that.

In fact, this probably shouldn't be a switch. It should probably be an abstract method or a function member on the enum itself. Switches on enums are a bit of a smell, and switches on enums from within the enum itself are so smelly as to almost be an error, IMHO. I realize you didn't add this switch, but this is a good opportunity to refactor it and leave it better than we found it.

maybe we can compromise on removing the default? 😅

not-napoleon · 2025-08-11T18:22:54Z

test/framework/src/main/java/org/elasticsearch/datageneration/MappingGenerator.java

@@ -64,7 +64,7 @@ public Mapping generate(Template template) {

        rawMapping.put("_doc", topLevelMappingParameters);

-        if (specification.fullyDynamicMapping()) {
+        if (specification.fullyDynamicMapping() == false) {


Why are we inverting this check? Especially while leaving a comment that says the thing we just checked was false must be true?

I fixed the comment. Note how the code path is inverted and the actual setup for dynamic mapping is below

...c/main/java/org/elasticsearch/datageneration/datasource/DefaultMappingParametersHandler.java

not-napoleon · 2025-08-11T18:43:28Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+    private List<XContentBuilder> documents = null;
+    private DataGenerationHelper dataGenerationHelper;
+
+    private static final class DataGenerationHelper {


I wonder if this should be a top level class. Seems like we'll want to build multiple test classes using this framework.

I've moved this class to its own file! TY.

not-napoleon · 2025-08-11T18:50:17Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+        private static Object randomDimensionValue(String dimensionName) {
+            // We use dimensionName to determine the type of the value.
+            var isNumeric = dimensionName.hashCode() % 5 == 0;
+            if (isNumeric) {


What about IP dimensions?

added 20% of dimensions as IP-like.

as follow up ill add dynamic mapping to parse as ip. thoughts?

Why not rely on the existing one:
https://github.com/elastic/elasticsearch/blob/main/test/framework/src/main/java/org/elasticsearch/datageneration/fields/leaf/IpFieldDataGenerator.java

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

pabloem

TY @not-napoleon - ptal!

pabloem · 2025-08-11T23:40:46Z

test/framework/src/main/java/org/elasticsearch/datageneration/MappingGenerator.java

@@ -64,7 +64,7 @@ public Mapping generate(Template template) {

        rawMapping.put("_doc", topLevelMappingParameters);

-        if (specification.fullyDynamicMapping()) {
+        if (specification.fullyDynamicMapping() == false) {


I fixed the comment. Note how the code path is inverted and the actual setup for dynamic mapping is below

...c/main/java/org/elasticsearch/datageneration/datasource/DefaultMappingParametersHandler.java

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

pabloem · 2025-08-12T00:03:53Z

test/framework/src/main/java/org/elasticsearch/datageneration/FieldType.java

@@ -78,6 +79,7 @@ public FieldDataGenerator generator(String fieldName, DataSource dataSource) {
            case IP -> new IpFieldDataGenerator(dataSource);
            case CONSTANT_KEYWORD -> new ConstantKeywordFieldDataGenerator();
            case WILDCARD -> new WildcardFieldDataGenerator(dataSource);
+            default -> null;


maybe we can compromise on removing the default? 😅

pabloem · 2025-08-12T00:04:23Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+    private List<XContentBuilder> documents = null;
+    private DataGenerationHelper dataGenerationHelper;
+
+    private static final class DataGenerationHelper {


I've moved this class to its own file! TY.

pabloem · 2025-08-12T00:06:49Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+        private static Object randomDimensionValue(String dimensionName) {
+            // We use dimensionName to determine the type of the value.
+            var isNumeric = dimensionName.hashCode() % 5 == 0;
+            if (isNumeric) {


added 20% of dimensions as IP-like.

pabloem · 2025-08-12T00:17:43Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+        private static Object randomDimensionValue(String dimensionName) {
+            // We use dimensionName to determine the type of the value.
+            var isNumeric = dimensionName.hashCode() % 5 == 0;
+            if (isNumeric) {


as follow up ill add dynamic mapping to parse as ip. thoughts?

kkrik-es · 2025-08-12T07:39:36Z

...rc/main/java/org/elasticsearch/datageneration/datasource/DefaultObjectGenerationHandler.java

-        );
+        return new DataSourceResponse.FieldTypeGenerator(() -> {
+            // All field types minus the excluded ones.
+            var fieldTypes = Arrays.stream(FieldType.values())


Nit: store this in a static variable.

kkrik-es · 2025-08-12T07:40:51Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+import static org.hamcrest.Matchers.closeTo;
+import static org.hamcrest.Matchers.equalTo;
+
+public class GenerativeTSIT extends AbstractEsqlIntegTestCase {


Nit: let's expand to GenerativeTimeSeriesIT. Also, maybe skip Generative for now, since this is different than the other generative tests.

kkrik-es · 2025-08-12T07:42:53Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+                )
+                .filter(val -> val.v2().isEmpty() == false) // Filter out empty values
+                .map(tup -> tup.v1() + ":" + tup.v2())
+                .toList();


Nit: maybe it's just me, but the above is somewhat hard to parse. I'd move this to a helper function.

kkrik-es · 2025-08-12T07:44:00Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+
+    static Long windowStart(Object timestampCell, int secondsInWindow) {
+        // The timestamp is in the 4th column (index 3)
+        return Instant.parse((String) timestampCell).toEpochMilli() / 1000 / secondsInWindow * secondsInWindow;


Why do we have / secondsInWindow * secondsInWindow?

Let's also use parentheses for readability.

kkrik-es · 2025-08-12T07:46:18Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+        Settings.Builder settingsBuilder = Settings.builder();
+        // Ensure it will be a TSDB data stream
+        settingsBuilder.put(IndexSettings.MODE.getKey(), IndexMode.TIME_SERIES);
+        settingsBuilder.putList("index.routing_path", List.of("attributes.*"));


No need to define this for data streams, dimensions are automatically added to the routing path.

kkrik-es · 2025-08-12T07:47:28Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+        settingsBuilder.put(IndexSettings.MODE.getKey(), IndexMode.TIME_SERIES);
+        settingsBuilder.putList("index.routing_path", List.of("attributes.*"));
+        CompressedXContent mappings = mappingString == null ? null : CompressedXContent.fromJSON(mappingString);
+        // print the mapping


Print? Sorry I missed this, what does this mean?

kkrik-es · 2025-08-12T07:52:16Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+                    .map(doc -> ((Map<String, Integer>) doc.get("metrics")).get("gauge_hdd.bytes.used"))
+                    .toList();
+                // Verify that the first column is the max value (the query gets max, avg, min in that order)
+                docValues.stream().max(Integer::compareTo).ifPresentOrElse(maxValue -> {


Same, let's move calculation of the expected values to separate functions so that they can be generalized later.

kkrik-es · 2025-08-12T07:54:40Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+     * there is only one metric group per time bucket.
+     */
+    public void testGroupByNothing() {
+        try (EsqlQueryResponse resp = run(String.format(Locale.ROOT, """


There's a lot of duplication between these tests. Let's try to refactor them and move shared parts to utility functions.

kkrik-es · 2025-08-12T07:57:22Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+                var docValues = windowDataPoints.stream()
+                    .map(doc -> ((Map<String, Integer>) doc.get("metrics")).get("gauge_hdd.bytes.used"))
+                    .toList();
+                // Verify that the first column is the max value (the query gets max, avg, min in that order)


In a follow-up, you can avoid hard-coding by defining an enum for each fuction, with corresponding validation logic.

kkrik-es · 2025-08-12T07:58:45Z

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java

+                // Verify that the second column is the avg value (thus why row.get(2))
+                docValues.stream().mapToDouble(Integer::doubleValue).average().ifPresentOrElse(avgValue -> {
+                    var res = (Double) row.get(2);
+                    assertThat(res, closeTo(avgValue, res * 0.5));


Why do we need the 0.5 factor?

kkrik-es · 2025-08-12T08:00:35Z

...src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/TSDataGenerationHelper.java

+
+    private static Object randomDimensionValue(String dimensionName) {
+        // We use dimensionName to determine the type of the value.
+        var isNumeric = dimensionName.hashCode() % 5 == 0;


Nit: let's just use randomDouble() < 0.2. Same below.

I see, this needs to be consistent per field. Why not rely on the data generation framework to provide these values? You can start simple with all dimensions being keyword fields - not dynamic, no pass-through subfields.

kkrik-es · 2025-08-12T08:01:36Z

...src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/TSDataGenerationHelper.java

+        var isNumeric = dimensionName.hashCode() % 5 == 0;
+        var isIP = dimensionName.hashCode() % 5 == 1;
+        if (isNumeric) {
+            // Numeric values are sometimes passed as integers and sometimes as strings.


We don't need to test parsing here so let's just use one of the formats.

kkrik-es · 2025-08-12T08:02:18Z

...src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/TSDataGenerationHelper.java

+        this.numDocs = numDocs;
+        attributesForMetrics = List.copyOf(Set.copyOf(ESTestCase.randomList(1, 300, () -> ESTestCase.randomAlphaOfLengthBetween(2, 30))));
+        numTimeSeries = ESTestCase.randomIntBetween(10, (int) Math.sqrt(numDocs));
+        // System.out.println("Total of time series: " + numTimeSeries);


kkrik-es · 2025-08-12T08:19:15Z

...src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/TSDataGenerationHelper.java

+                Map.of(
+                    "gauge_double",
+                    Map.of("path_match", "metrics.gauge_*", "mapping", Map.of("type", "double", "time_series_metric", "gauge"))
+                )


The last two have the same path as the above, I'm surprised that this is allowed. Let's skip them for now, since you only use one long gauge.

kkrik-es

Thanks Pablo, this ia a good step. It's nice that you tried to include the pass-through field on the first take, though that complicates things somewhat. I'd start with statically defined dimension and metric fields to get the validation logic in place first, then introduce dynamic fields on top of that.

Let's try to refactor the logic slightly so that it can be further extended in follow-up PRs.

First test case in prototype messy test file

613bda8

elasticsearchmachine added the v9.2.0 label Aug 7, 2025

elasticsearchmachine and others added 6 commits August 8, 2025 00:04

[CI] Auto commit changes from spotless

2d70cef

First two randomized test cases

19e2f8a

smol cleanup

14b4c17

Merge branch 'main' into pem-randomrandom-testing

836d49a

[CI] Auto commit changes from spotless

64624be

cleanup and ready for first check

77a368b

pabloem marked this pull request as ready for review August 11, 2025 18:06

Merge branch 'main' into pem-randomrandom-testing

f53f904

pabloem changed the title ~~[wip][do not review] A random-random test for time-series data~~ A random-random test for time-series data Aug 11, 2025

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Aug 11, 2025

pabloem added >test Issues or PRs that are addressing/adding tests :StorageEngine/TSDB You know, for Metrics :StorageEngine/ES|QL Timeseries / metrics / logsdb capabilities in ES|QL and removed needs:triage Requires assignment of a team area label labels Aug 11, 2025

elasticsearchmachine added the Team:StorageEngine label Aug 11, 2025

not-napoleon reviewed Aug 11, 2025

View reviewed changes

Address comments

afdac85

pabloem commented Aug 12, 2025

View reviewed changes

kkrik-es reviewed Aug 12, 2025

View reviewed changes

A random-random test for time-series data #132556

Are you sure you want to change the base?

A random-random test for time-series data #132556

Uh oh!

Conversation

pabloem commented Aug 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Aug 11, 2025

Uh oh!

not-napoleon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pabloem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pabloem commented Aug 7, 2025 •

edited

Loading