Fix downsampling with disabled subobjects by gmarouli · Pull Request #138715 · elastic/elasticsearch

gmarouli · 2025-11-27T09:29:31Z

The downsampling operation is editing the source mapping to replace gauge metrics with an aggregate type which contains the pre-aggregated data.

However, the process does not respect the disabled subobjects configured by parent fields, for example, a passthrough field and gets conflicts as it tried to reconstruct the updated mapping.

For example, the following valid mapping section:

     "metrics": {
        "type": "passthrough",
        "priority": 10,
        "dynamic": "true",
        "properties": {
          "k8s.volume.inodes": {
            "type": "long",
            "time_series_metric": "gauge"
          },
          "k8s.volume.inodes.free": {
            "type": "long",
            "time_series_metric": "gauge"
          },
          "k8s.volume.inodes.used": {
            "type": "long",
            "time_series_metric": "gauge"
          }
        }
      }

gets converted to:

          "metrics.k8s.volume.inodes": {
            "type": "aggregate_metric_double",
            "time_series_metric": "gauge",
            ....
          },
          "metrics.k8s.volume.inodes.free": {
            "type": "aggregate_metric_double",
            "time_series_metric": "gauge",
          ...
          },
          "metrics.k8s.volume.inodes.used": {
            "type": "aggregate_metric_double",
            "time_series_metric": "gauge",
            ...
          }

Which is not valid because the disabled subobjects is not set anymore.

This PR (re)proposes to use the visitor pattern to update the source mapping in place. Last time we proposed that there was a better solution; however, now we see that the original structure of the mapping is important for inherited properties.

Fixes: #138572

elasticsearchmachine · 2025-11-27T09:29:56Z

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

elasticsearchmachine · 2025-11-27T09:30:21Z

Hi @gmarouli, I've created a changelog YAML for you.

server/src/test/java/org/elasticsearch/action/admin/cluster/stats/MappingVisitorTests.java

...n/downsample/src/main/java/org/elasticsearch/xpack/downsample/TransportDownsampleAction.java

kkrik-es · 2025-11-27T11:45:15Z

...n/downsample/src/main/java/org/elasticsearch/xpack/downsample/TransportDownsampleAction.java

+        Map<String, Object> downsampledMapping = new HashMap<>();
+        for (Map.Entry<String, Object> entry : sourceIndexMappings.entrySet()) {
+            if (entry.getKey().equals(PROPERTIES) == false) {
+                downsampledMapping.put(entry.getKey(), entry.getValue());


Why do we need this? Maybe add a comment?

Added a comment and a test to ensure this will not be accidentally removed.

kkrik-es · 2025-11-27T11:52:30Z

server/src/test/java/org/elasticsearch/action/admin/cluster/stats/MappingVisitorTests.java

+        );
+
+        HashMap<String, Object> result = new HashMap<>();
+        MappingVisitor.visitAndCopyMapping(expectedProperties, result, (ignored, source, dest) -> {


Shall we also add a test that creates random mappings and checks that the copied one is a clone of the source?

Great idea!

Let's add this separately.

…ats/MappingVisitorTests.java Co-authored-by: Kostas Krikellas <131142368+kkrik-es@users.noreply.github.com>

gmarouli · 2025-11-28T09:07:37Z

Update:

I am currently working on the ci failures. The problem is that we need to be able to distringuish and handle differently a multi field from a subobjects false field :) Working on it.

gmarouli · 2025-12-02T12:04:42Z

Waiting for #138869

gmarouli · 2025-12-05T15:09:47Z

server/src/test/java/org/elasticsearch/action/admin/cluster/stats/MappingVisitorTests.java

+    @SuppressWarnings("unchecked")
+    public void testVisitAndCopy() {
+        DataGeneratorSpecification specification = DataGeneratorSpecification.buildDefault();
+        var template = new TemplateGenerator(specification).generate();
+        MappingGenerator mappingGenerator = new MappingGenerator(specification);
+        for (int i = 0; i < MAPPING_GENERATION_ROUNDS; i++) {
+            var mapping = mappingGenerator.generate(template).raw();
+            var properties = (Map<String, Object>) mapping.get("_doc");
+            var updatedMapping = new HashMap<String, Object>();
+            MappingVisitor.visitPropertiesAndCopyMapping(properties, updatedMapping, (f, source, dest) -> dest.put(f, source));
+            assertThat(updatedMapping, equalTo(updatedMapping));
+        }
+    }


@kkrik-es oh I did add this one, is this not what you had in mind?

elasticsearchmachine · 2025-12-05T15:14:00Z

💔 Backport failed

Status	Branch	Result
❌	9.2	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 138715

gmarouli · 2025-12-05T15:32:43Z

💚 All backports created successfully

Status	Branch	Result
✅	9.2

Questions ?

Please refer to the Backport tool documentation

We change the way the downsampled mapping gets created. Instead of creating a new mapping with the metric fields and then merging it with the original, we use a visitor to copy and change the original. (cherry picked from commit c2e6a67) # Conflicts: # x-pack/plugin/downsample/src/main/java/org/elasticsearch/xpack/downsample/TransportDownsampleAction.java # x-pack/plugin/downsample/src/test/java/org/elasticsearch/xpack/downsample/TransportDownsampleActionTests.java

We change the way the downsampled mapping gets created. Instead of creating a new mapping with the metric fields and then merging it with the original, we use a visitor to copy and change the original.

We change the way the downsampled mapping gets created. Instead of creating a new mapping with the metric fields and then merging it with the original, we use a visitor to copy and change the original. (cherry picked from commit c2e6a67) # Conflicts: # x-pack/plugin/downsample/src/main/java/org/elasticsearch/xpack/downsample/TransportDownsampleAction.java # x-pack/plugin/downsample/src/test/java/org/elasticsearch/xpack/downsample/TransportDownsampleActionTests.java

We change the way the downsampled mapping gets created. Instead of creating a new mapping with the metric fields and then merging it with the original, we use a visitor to copy and change the original.

gmarouli added 3 commits November 27, 2025 10:17

Add test to capture the issue with mapping with disabled subobjects

d82f207

Add visitor infrastructure

592d8fb

Update the mapping

691bb49

gmarouli requested a review from kkrik-es November 27, 2025 09:29

gmarouli added >bug auto-backport Automatically create backport pull requests when merged :StorageEngine/Downsampling Downsampling (replacement for rollups) - Turn fine-grained time-based data into coarser-grained data branch:9.2 labels Nov 27, 2025

elasticsearchmachine added the Team:StorageEngine label Nov 27, 2025

elasticsearchmachine added v9.3.0 v9.2.3 and removed branch:9.2 labels Nov 27, 2025

Update docs/changelog/138715.yaml

c7189f5

kkrik-es reviewed Nov 27, 2025

View reviewed changes

server/src/test/java/org/elasticsearch/action/admin/cluster/stats/MappingVisitorTests.java Outdated Show resolved Hide resolved

kkrik-es reviewed Nov 27, 2025

View reviewed changes

...n/downsample/src/main/java/org/elasticsearch/xpack/downsample/TransportDownsampleAction.java Show resolved Hide resolved

kkrik-es reviewed Nov 27, 2025

View reviewed changes

gmarouli and others added 2 commits November 28, 2025 09:37

Update server/src/test/java/org/elasticsearch/action/admin/cluster/st…

11b9606

…ats/MappingVisitorTests.java Co-authored-by: Kostas Krikellas <131142368+kkrik-es@users.noreply.github.com>

Fix test

1de43d6

gmarouli added 6 commits December 4, 2025 16:48

merge with main

d6790c8

Do not "visit" multi-fields just properties

6dd0a78

Test new visitor with random mappings

b056826

Fix test

a774d28

Merge branch 'main' into fix-downsampling-with-disabled-subobjects

c8f210e

Test the downsampled mapping

0374587

gmarouli requested a review from kkrik-es December 5, 2025 14:48

kkrik-es approved these changes Dec 5, 2025

View reviewed changes

gmarouli commented Dec 5, 2025

View reviewed changes

gmarouli merged commit c2e6a67 into elastic:main Dec 5, 2025
34 checks passed

gmarouli deleted the fix-downsampling-with-disabled-subobjects branch December 5, 2025 15:12

elasticsearchmachine added the backport pending label Dec 5, 2025

gmarouli mentioned this pull request Dec 5, 2025

[9.2] Fix downsampling with disabled subobjects (#138715) #139133

Merged

gmarouli mentioned this pull request Jan 31, 2026

[8.19] Fix downsampling with disabled subobjects (#138715) #141622

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix downsampling with disabled subobjects#138715

Fix downsampling with disabled subobjects#138715
gmarouli merged 12 commits intoelastic:mainfrom
gmarouli:fix-downsampling-with-disabled-subobjects

gmarouli commented Nov 27, 2025

elasticsearchmachine commented Nov 27, 2025

elasticsearchmachine commented Nov 27, 2025

Uh oh!

Uh oh!

kkrik-es Nov 27, 2025

gmarouli Dec 5, 2025

kkrik-es Nov 27, 2025

gmarouli Nov 27, 2025

kkrik-es Dec 5, 2025

gmarouli commented Nov 28, 2025

gmarouli commented Dec 2, 2025

gmarouli Dec 5, 2025

Uh oh!

elasticsearchmachine commented Dec 5, 2025

gmarouli commented Dec 5, 2025

Labels

3 participants

Conversation

gmarouli commented Nov 27, 2025

elasticsearchmachine commented Nov 27, 2025

elasticsearchmachine commented Nov 27, 2025

Uh oh!

Uh oh!

kkrik-es Nov 27, 2025

Choose a reason for hiding this comment

gmarouli Dec 5, 2025

Choose a reason for hiding this comment

kkrik-es Nov 27, 2025

Choose a reason for hiding this comment

gmarouli Nov 27, 2025

Choose a reason for hiding this comment

kkrik-es Dec 5, 2025

Choose a reason for hiding this comment

gmarouli commented Nov 28, 2025

gmarouli commented Dec 2, 2025

gmarouli Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Dec 5, 2025

💔 Backport failed

gmarouli commented Dec 5, 2025

💚 All backports created successfully

Questions ?

Labels

3 participants