Add `synthetic_source` support to `aggregate_metric_double` fields #88909

csoulios · 2022-07-28T16:00:57Z

This PR implements synthetic_source support to the aggregate_metric_double field type

Relates to #86603

elasticsearchmachine · 2022-07-28T16:02:50Z

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

elasticsearchmachine · 2022-07-28T16:38:45Z

Pinging @elastic/es-search (Team:Search)

csoulios · 2022-07-28T16:44:59Z

...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java

+        public Leaf leaf(LeafReader reader, int[] docIdsInLeaf) throws IOException {
+            Map<Metric, SortedNumericDocValues> metricDocValues = new EnumMap<>(Metric.class);
+            for (Metric m : metrics) {
+                SortedNumericDocValues dv = dv(reader, m);
+                if (dv != null) {
+                    metricDocValues.put(m, dv);
+                }
+            }
+
+            if (metricDocValues.isEmpty()) {
+                return SourceLoader.SyntheticFieldLoader.NOTHING_LEAF;
+            }
+
+            return new AggregateMetricSyntheticFieldLoader.ImmediateLeaf(metricDocValues);
+        }


I could not find an easy way to leverage the NumericSyntheticFieldLoader class, so I had to rewrite the same implementation so that it uses one SortedNumericDocValues instance per metric.

I skipped the singleton optimization because having mutliple docs in leaf requires an array of values. This combined with multiple metrics per doc (a Map<Metric, SortedNumericDocValues> will make the code extremely complex. I can do it if we think this optimization is worth the complexity

csoulios · 2022-07-28T16:57:31Z

...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java

+            NumericDocValues single = reader.getNumericDocValues(fieldName);
+            if (single != null) {
+                return DocValues.singleton(single);
+            }
+            return null;


Since all metric subfields are stored using the SortedNumericDocValuesField, all doc values will be stored as SortedNumericDocValues. So, I am not sure reader.getNumericDocValues(fieldName) will ever return non-null values

Try removing it! I believe if you store single values it'll flip to NumericDocValues anyway.

If you do have to keep loading from both of there it's probably worth making this method static and public in NumericFieldMapper or something like that.

csoulios · 2022-07-28T17:03:03Z

...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java

+                    return;
+                }
+                b.startObject(simpleName);
+                for (Map.Entry<Metric, SortedNumericDocValues> entry : metricDocValues.entrySet()) {


metricDocValues is an EnumMap instance. This means that the order that sub fields will be returned in the source, will be the order in the Metric enum. Order ismin,max, sum, value_count. It is not alphabetical, but it will always be consistent. Does it make sense to change this so subfields are retuned in an alphabetical order?

It's cool. Consistency is lovely for reading these docs. Alphabetical just gets the consistency in an easy way. No need to do it here.

nik9000

I left a few questions. I think this works fine though! I'm just poking around the edges, suggesting some maybe silly ideas. Either way, I think it's right.

nik9000 · 2022-07-29T12:35:22Z

...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java

@@ -292,6 +296,7 @@ private NumberFieldMapper.NumberFieldType delegateFieldType(Metric metric) {

        /**
         * Return a delegate field type for the default metric sub-field
+         *


Does your formatter do this? I think it's more standard, but I don't tend to do it myself.

If I undertand the question well, this has nothing to do with formatting the field in the output.

This has to do with the following:

Querying the field: When running a term or a range query, the query is delegated to the subfield metric that has been marked as default_metric in the field mapping.

When retrieving the field using the fields api, the value of the default_metric is returned (this will change when Support aggregate_double_metric fields in the Field API #88534 is merged)

Sorry, no, my question was about the extra line inserted into the javadoc.

Sorry! I wasn't clear.

Looks like it was me messing with IntelliJ formatting. Reverted it now.

nik9000 · 2022-07-29T12:39:08Z

...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java

+            NumericDocValues single = reader.getNumericDocValues(fieldName);
+            if (single != null) {
+                return DocValues.singleton(single);
+            }
+            return null;


Try removing it! I believe if you store single values it'll flip to NumericDocValues anyway.

If you do have to keep loading from both of there it's probably worth making this method static and public in NumericFieldMapper or something like that.

nik9000 · 2022-07-29T12:40:24Z

...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java

+                    return;
+                }
+                b.startObject(simpleName);
+                for (Map.Entry<Metric, SortedNumericDocValues> entry : metricDocValues.entrySet()) {


It's cool. Consistency is lovely for reading these docs. Alphabetical just gets the consistency in an easy way. No need to do it here.

...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java

csoulios · 2022-07-29T19:57:23Z

@elasticmachine run elasticsearch-ci/packaging-tests-unix-sample

csoulios · 2022-08-01T09:01:55Z

@elasticmachine update branch

nik9000

I left a tiny thing about switching types on the tracked map. Otherwise LGTM.

nik9000 · 2022-08-01T15:48:37Z

...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java

+
+            @Override
+            public boolean empty() {
+                return metricDocValues.isEmpty();


This is never empty, right? You could maybe just return false here. And In the ctor you could assert metricDocValues.empty == false

nik9000 · 2022-08-01T15:49:53Z

...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java

+                // it is enough to check for the first docValue. However, in the future
+                // we may relax the requirement of all metrics existing. In this case
+                // we should check the doc value for each metric separately
+                dvHasValue = new EnumMap<>(Metric.class);


Could you use an EnumSet instead? And maybe clear it instead of recreate it?

nik9000 · 2022-08-01T15:50:19Z

...in/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java

+                    dvHasValue.put(e.getKey(), leafHasValues);
+                }
+
+                return hasValues;


I think maybe dvHasValue.isEmpty() == false is enough here?

Add synthetic_source to aggregate_metric_double

0497813

csoulios added >enhancement :StorageEngine/TSDB You know, for Metrics v8.5.0 labels Jul 28, 2022

csoulios and others added 2 commits July 28, 2022 19:02

Update docs/changelog/88909.yaml

7571ad4

Added docs

4e9f616

csoulios added :Search Foundations/Mapping Index mappings, including merging and defining field types and removed :StorageEngine/TSDB You know, for Metrics labels Jul 28, 2022

csoulios added 2 commits July 28, 2022 19:22

Minor changes

8d62217

Removed the singleton optimization

f2c02d8

csoulios marked this pull request as ready for review July 28, 2022 16:38

csoulios requested a review from nik9000 July 28, 2022 16:38

elasticsearchmachine added the Team:Search Meta label for search team label Jul 28, 2022

csoulios commented Jul 28, 2022

View reviewed changes

Merge branch 'main' into agg-metric-synthetic-src

86196bc

nik9000 reviewed Jul 29, 2022

View reviewed changes

csoulios added 4 commits July 29, 2022 16:33

use dv() method from NumberFieldMapper

24e9bba

Leaf is empty if there are no doc values

bef49b4

checkstyle

34ad768

Changed formatting

faaa6c1

Merge branch 'main' into agg-metric-synthetic-src

e5c18ad

csoulios mentioned this pull request Aug 1, 2022

Synthetic Source #86603

Closed

50 tasks

csoulios requested a review from nik9000 August 1, 2022 09:40

nik9000 approved these changes Aug 1, 2022

View reviewed changes

csoulios added 3 commits August 1, 2022 19:08

Merge branch 'main' into agg-metric-synthetic-src

0a9322a

small improvements

97dcc6d

Merge branch 'main' into agg-metric-synthetic-src

70dd5a3

csoulios merged commit ad2dc83 into elastic:main Aug 1, 2022

csoulios deleted the agg-metric-synthetic-src branch August 1, 2022 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `synthetic_source` support to `aggregate_metric_double` fields #88909

Add `synthetic_source` support to `aggregate_metric_double` fields #88909

csoulios commented Jul 28, 2022 •

edited

Loading

elasticsearchmachine commented Jul 28, 2022

elasticsearchmachine commented Jul 28, 2022

csoulios Jul 28, 2022

csoulios Jul 28, 2022 •

edited

Loading

nik9000 Jul 29, 2022

csoulios Jul 28, 2022 •

edited

Loading

nik9000 Jul 29, 2022

nik9000 left a comment

nik9000 Jul 29, 2022

csoulios Jul 29, 2022

nik9000 Jul 29, 2022

nik9000 Jul 29, 2022

csoulios Jul 29, 2022

nik9000 Jul 29, 2022

nik9000 Jul 29, 2022

csoulios commented Jul 29, 2022

csoulios commented Aug 1, 2022

nik9000 left a comment

nik9000 Aug 1, 2022

nik9000 Aug 1, 2022

nik9000 Aug 1, 2022

Add synthetic_source support to aggregate_metric_double fields #88909

Add synthetic_source support to aggregate_metric_double fields #88909

Conversation

csoulios commented Jul 28, 2022 • edited Loading

elasticsearchmachine commented Jul 28, 2022

elasticsearchmachine commented Jul 28, 2022

Choose a reason for hiding this comment

csoulios Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csoulios Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csoulios commented Jul 29, 2022

csoulios commented Aug 1, 2022

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `synthetic_source` support to `aggregate_metric_double` fields #88909

Add `synthetic_source` support to `aggregate_metric_double` fields #88909

csoulios commented Jul 28, 2022 •

edited

Loading

csoulios Jul 28, 2022 •

edited

Loading

csoulios Jul 28, 2022 •

edited

Loading