Normalize malformed aggregate_double_metric by nik9000 · Pull Request #90815 · elastic/elasticsearch

nik9000 · 2022-10-11T18:43:27Z

This makes ignore_malformed for aggregate_double_metric save just the top level field name into the meta _ignored field, like every other field. It does so by pulling part of the parsing logic back from it's delegate numeric fields. Now it uses the delegates to parse the value and to save the values, but not to handle malformed values. Instead, it handles malformed values itself on the top level.

Doing this made it fairly simple to solve two other issues:

ignore_malformed had to remove non-malformed values that had previously been added to the document. Now we only save the values after all have been parsed.
When we validate some of the values we use to have to dig them out of the document using inefficient code that was written mostly for testing. Now that we're saving the values before adding them to the document it's easy enough to validate from that list instead which amounts to a single array access.

Closes #90572

nik9000 · 2022-10-11T18:43:52Z

...va/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapperTests.java

-            e.getCause().getMessage(),
-            containsString("failed to parse field [field.value_count] of type [integer] in document with id '1'.")
-        );
-    }


This is tested as part of the exampleMalformedValues.

nik9000 · 2022-10-11T18:44:37Z

server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java

-    private static Number value(XContentParser parser, NumberType numberType, Number nullValue, boolean coerce)
-        throws IllegalArgumentException, IOException {
-
+    public Number value(XContentParser parser) throws IllegalArgumentException, IOException {


This'll need javadoc if we want to keep it. It's sort of leaky, but I've done worse. It's for a good cause.

This is leaky indeed, but I subscribe to your good cause. Not sure if there's a better way to parse and index the value. If the es-search is ok with it, I am fine too

Maybe @romseygeek should have a look here too.

Yep fine by me.

This makes `ignore_malformed` for `aggregate_double_metric` save just the top level field name into the meta `_ignored` field, like every other field. It does so by pulling part of the parsing logic back from it's delegate numeric fields. Now it uses the delegates to parse the value and to save the values, but not to handle malformed values. Instead, it handles malformed values itself on the top level. Doing this made it fairly simple to solve two other issues: * `ignore_malformed` had to remove non-malformed values that had previously been added to the document. Now we only save the values after all have been parsed. * When we validate some of the values we use to have to dig them out of the document using inefficient code that was written mostly for testing. Now that we're saving the values before adding them to the document it's easy enough to validate from that list instead which amounts to a single array access. Closes elastic#90572

elasticsearchmachine · 2022-10-12T17:50:54Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

csoulios

I think the big dilemma here is if we want to expose the parse/index methods of the NumberFieldMapper class. I don't see why not to.

I have left some small comments.

csoulios · 2022-10-19T12:50:52Z

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

+                    someMalformedValues = true;
+                }
+            }
+            MalformedHandler onMalformed = new MalformedHandler();


I don't understand where exactly is this MalformedHandler used.

Uh..... Its been a while since I wrote this and I can't find it. Maybe it's a leftover of another attempt. I'll get back to you.

csoulios · 2022-10-19T12:55:45Z

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

            }
+
+            // Check if all required metrics have been parsed.
+            for (Metric m : metrics) {


What if you only checked the sizes of metrics and metricsParsed. You would not have to iterate over metrics?
You think there can be a case that this fails?

csoulios · 2022-10-19T13:00:05Z

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

+            // Check that there aren't any duplicates already parsed
+            for (Metric m : metricsParsed.keySet()) {
+                NumberFieldMapper delegateFieldMapper = metricFieldMappers.get(m);
+                if (context.doc().getByKey(delegateFieldMapper.fieldType().name()) != null) {


Method Document.onlyAddKey() was supposed to check for duplicate fields in the doc. I think this method is called from NumferFieldMapper.indexValue(). In this case the duplicates check is performed twice.

Yeah. I suppose I don't need to check this. It's just passing the right exception message back. I think don't need to check this because a duplicate will fail with the first key every time - because we don't index unless all keys are present. That's what I did above.

I think this method deserves another visit because I see us constantly doing a test like this one to get a better error message. I'd like to smooth it a bit in a follow up if that's ok
.

csoulios · 2022-10-19T13:02:05Z

server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java

-    private static Number value(XContentParser parser, NumberType numberType, Number nullValue, boolean coerce)
-        throws IllegalArgumentException, IOException {
-
+    public Number value(XContentParser parser) throws IllegalArgumentException, IOException {


This is leaky indeed, but I subscribe to your good cause. Not sure if there's a better way to parse and index the value. If the es-search is ok with it, I am fine too

romseygeek

This makes sense to me, happy to have another look when @csoulios 's feedback has been addressed.

romseygeek · 2022-10-31T15:22:34Z

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

+                return;
            }
+            // Rethrow exception as is. It is going to be caught and nested in a MapperParsingException
+            // by its FieldMapper.MappedFieldType#parse()


Just FieldMapper#parse I think?

This comment still applies

romseygeek · 2022-10-31T15:24:42Z

server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java

-    private static Number value(XContentParser parser, NumberType numberType, Number nullValue, boolean coerce)
-        throws IllegalArgumentException, IOException {
-
+    public Number value(XContentParser parser) throws IllegalArgumentException, IOException {


Yep fine by me.

romseygeek

One nit in a comment, LGTM otherwise

romseygeek · 2022-11-04T14:34:40Z

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

+                return;
            }
+            // Rethrow exception as is. It is going to be caught and nested in a MapperParsingException
+            // by its FieldMapper.MappedFieldType#parse()


This comment still applies

csoulios

LGTM!

nik9000 added >non-issue :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data v8.6.0 labels Oct 11, 2022

nik9000 requested a review from csoulios October 11, 2022 18:43

nik9000 commented Oct 11, 2022

View reviewed changes

nik9000 requested a review from romseygeek October 11, 2022 18:44

nik9000 marked this pull request as ready for review October 12, 2022 17:50

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 12, 2022

csoulios reviewed Oct 19, 2022

View reviewed changes

romseygeek reviewed Oct 31, 2022

View reviewed changes

Merge branch 'main' into aggregate_metric_double_ignores

6a215ef

nik9000 requested a review from csoulios November 3, 2022 16:49

Cleanup

6e343b2

nik9000 requested review from csoulios and romseygeek and removed request for csoulios November 4, 2022 14:30

romseygeek approved these changes Nov 4, 2022

View reviewed changes

csoulios approved these changes Nov 7, 2022

View reviewed changes

nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 7, 2022

nik9000 added 2 commits November 7, 2022 10:04

Merge branch 'main' into aggregate_metric_double_ignores

ea7d50a

Wrong name

d391d66

elasticsearchmachine merged commit d29d2e8 into elastic:main Nov 7, 2022

nik9000 deleted the aggregate_metric_double_ignores branch November 7, 2022 15:31

Conversation

nik9000 commented Oct 11, 2022

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!

elasticsearchmachine commented Oct 12, 2022

Uh oh!

csoulios 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!

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!

romseygeek 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!

romseygeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csoulios left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants