Fix Unknown NamedWriteable error in InternalAggregation (Add extra round trip to aggs tests) #79638

nik9000 · 2021-10-21T15:56:10Z

This tries to automatically detect problems seriealization aggregation
results by round tripping the results in our usual AggregatorTestCase.
It's "free" testing in that we already have the tests written and we'll
get round trip testing "on the side". But it's kind of sneaky because we
aren't trying to test serialization here. So they failures can be
surprising. But surprising test failures are better than bugs. At least
that is what I tell myself so I can sleep at night.

EDIT:

This actually uncovered a real bug that'd show up as "Unknown NamedWriteable"
when using certain pipeline aggregations in async search. So I've marked this as
>bug. And will backport to 7.16.

Closes #73680

elasticmachine · 2021-10-21T15:56:15Z

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

This tries to automatically detect problems seriealization aggregation results by round tripping the results in our usual `AggregatorTestCase`. It's "free" testing in that we already have the tests written and we'll get round trip testing "on the side". But it's kind of sneaky because we aren't *trying* to test serialization here. So they failures can be surprising. But surprising test failures are better than bugs. At least that is what I tell myself so I can sleep at night. Closes elastic#73680

nik9000 · 2021-10-21T15:56:45Z

server/src/main/java/org/elasticsearch/search/DocValueFormat.java

@@ -317,6 +317,26 @@ public double parseDouble(String value, boolean roundUp, LongSupplier now) {
        public String toString() {
            return "DocValueFormat.DateTime(" + formatter + ", " + timeZone + ", " + resolution + ")";
        }
+
+        @Override
+        public boolean equals(Object o) {


We use actual date formats in the DateHistogramAggregatorTests and they didn't have equals.

zoinks. Nice catch.

I think it's only important for the tests. But that's ok.

nik9000 · 2021-10-21T15:57:22Z

...c/main/java/org/elasticsearch/search/aggregations/bucket/terms/InternalSignificantTerms.java

@@ -134,15 +134,14 @@ public boolean equals(Object o) {
            }

            Bucket<?> that = (Bucket<?>) o;
-            return bucketOrd == that.bucketOrd
-                && Double.compare(that.score, score) == 0
+            return Double.compare(that.score, score) == 0


bucketOrd is lost in serialization. It's a transient part of building the bucket and not really part of the identity.

Probably worth leaving as a comment on the member declaration.

nik9000 · 2021-10-21T16:25:20Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/InternalTerms.java

            return Objects.equals(docCount, that.docCount)
-                && Objects.equals(docCountError, that.docCountError)
+                && Objects.equals(showDocCountError, that.showDocCountError)
+                && Objects.equals(format, that.format)
                && Objects.equals(aggregations, that.aggregations);


Two things going on here:

docCountError is converted into -1 by the serialization process if showDocCountError is false. I looked into making it consistent but that seemed more complex than it was worth.

We may as well actually check the extra stuff. It doesn't cost us anything.

nik9000 · 2021-10-21T16:25:45Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalCardinality.java

@@ -111,6 +111,9 @@ public boolean equals(Object obj) {
        if (super.equals(obj) == false) return false;

        InternalCardinality other = (InternalCardinality) obj;
+        if (counts == null) {
+            return other.counts == null;
+        }


Sometimes it's null but we never handled that in equals/hashcode

nik9000 · 2021-10-21T16:26:03Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTopHits.java

@@ -206,7 +206,6 @@ public boolean equals(Object obj) {
            ScoreDoc otherDoc = other.topDocs.topDocs.scoreDocs[d];
            if (thisDoc.doc != otherDoc.doc) return false;
            if (Double.compare(thisDoc.score, otherDoc.score) != 0) return false;
-            if (thisDoc.shardIndex != otherDoc.shardIndex) return false;


The shardIndex is destroyed by serialization.

nik9000 · 2021-10-21T16:26:25Z

...a/org/elasticsearch/search/aggregations/bucket/histogram/InternalAutoDateHistogramTests.java

@@ -111,7 +111,7 @@ public void testGetAppropriateRoundingUsesCorrectIntervals() {
        long startingDate = randomLongBetween(0, utcMillis("2050-01-01"));
        RoundingInfo[] roundingInfos = AutoDateHistogramAggregationBuilder.buildRoundings(null, null);
        int roundingIndex = between(0, roundingInfos.length - 1);
-        DocValueFormat format = randomNumericDocValueFormat();
+        DocValueFormat format = randomDateDocValueFormat();


May as well use a proper doc value format now that I've made it work with tests. You'll see this in a bunch of places.

nik9000 · 2021-10-21T16:28:10Z

.../java/org/elasticsearch/search/aggregations/bucket/histogram/InternalDateHistogramTests.java

+            ZoneOffset.UTC,
+            Resolution.MILLISECONDS
+        );
+        return createTestInstance(name, metadata, subAggs, xContentCompatibleFormat);


This is kind of neat. Random doc value formats may generate json with duplicate keys which we can't parse. That's never been considered really a bug. Just, like, don't configure the agg that way. But random tests will and those are valid for serialization. But not valid for xcontent. We've had this createTestInstanceForXContent style for a while. So I extended it here.

yeah, testing against random formats is hard. I think this pattern continues to make sense.

nik9000 · 2021-10-21T16:28:40Z

...r/src/test/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorTests.java

@@ -433,11 +433,11 @@ public void testUnmappedWithBadMissingField() {
    public void testSubAggCollectsFromSingleBucketIfOneRange() throws IOException {
        RangeAggregationBuilder aggregationBuilder = new RangeAggregationBuilder("test").field(NUMBER_FIELD_NAME)
            .addRange(0d, 10d)
-            .subAggregation(aggCardinality("c"));
+            .subAggregation(aggCardinalityUpperBound("c"));


I renamed this because it was confusing to me when I bumped into it while fixing its serialization errors.

nik9000 · 2021-10-21T16:30:10Z

test/framework/src/main/java/org/elasticsearch/search/aggregations/AggregatorTestCase.java

+        protected void doWriteTo(StreamOutput out) throws IOException {
+            out.writeVInt(cardinality.map(i -> i));
+        }
+


We round trip this now.

nik9000 · 2021-10-21T16:30:38Z

test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java

+                originalBytes = toXContent(aggregation, xContentType, params, humanReadable);
+            }
+        } catch (IOException e) {
+            throw new IOException("error converting " + aggregation, e);


This error message was useless.

not-napoleon

LGTM.

not-napoleon · 2021-10-25T15:46:12Z

server/src/main/java/org/elasticsearch/search/DocValueFormat.java

@@ -317,6 +317,26 @@ public double parseDouble(String value, boolean roundUp, LongSupplier now) {
        public String toString() {
            return "DocValueFormat.DateTime(" + formatter + ", " + timeZone + ", " + resolution + ")";
        }
+
+        @Override
+        public boolean equals(Object o) {


zoinks. Nice catch.

not-napoleon · 2021-10-25T15:48:04Z

...c/main/java/org/elasticsearch/search/aggregations/bucket/terms/InternalSignificantTerms.java

@@ -134,15 +134,14 @@ public boolean equals(Object o) {
            }

            Bucket<?> that = (Bucket<?>) o;
-            return bucketOrd == that.bucketOrd
-                && Double.compare(that.score, score) == 0
+            return Double.compare(that.score, score) == 0


Probably worth leaving as a comment on the member declaration.

not-napoleon · 2021-10-25T15:54:11Z

.../java/org/elasticsearch/search/aggregations/bucket/histogram/InternalDateHistogramTests.java

+            ZoneOffset.UTC,
+            Resolution.MILLISECONDS
+        );
+        return createTestInstance(name, metadata, subAggs, xContentCompatibleFormat);


yeah, testing against random formats is hard. I think this pattern continues to make sense.

not-napoleon · 2021-10-25T16:00:55Z

test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java

@@ -526,9 +532,9 @@ public final void testFromXContentWithRandomFields() throws IOException {
    protected Predicate<String> excludePathsFromXContentInsertion() {
        return path -> false;
    }
-
+    


Nit: Trailing white space?

No idea. will zap.

not-napoleon · 2021-10-25T16:02:03Z

test/framework/src/main/java/org/elasticsearch/test/InternalAggregationTestCase.java

    /**
-     * @return a random {@link DocValueFormat} that can be used in aggregations which


I don't mind, but I'm curious why you don't think @return is appropriate here.

I think it looks better when I mouse over and get the javadoc. I think if we ever wanted to look at the html it'd be much better, but I never really do.

But it's really just a silly pet peeve of mine. I'll try to contain it.

This tries to automatically detect problems seriealization aggregation results by round tripping the results in our usual `AggregatorTestCase`. It's "free" testing in that we already have the tests written and we'll get round trip testing "on the side". But it's kind of sneaky because we aren't *trying* to test serialization here. So they failures can be surprising. But surprising test failures are better than bugs. At least that is what I tell myself so I can sleep at night. Closes elastic#73680

nik9000 · 2021-11-10T19:53:52Z

7.16 is #80632
8.1 is just this PR. We forked 8.1 after this.

This tries to automatically detect problems seriealization aggregation results by round tripping the results in our usual `AggregatorTestCase`. It's "free" testing in that we already have the tests written and we'll get round trip testing "on the side". But it's kind of sneaky because we aren't *trying* to test serialization here. So they failures can be surprising. But surprising test failures are better than bugs. At least that is what I tell myself so I can sleep at night. Closes #73680

nik9000 added >test Issues or PRs that are addressing/adding tests :Analytics/Aggregations Aggregations v8.0.0 labels Oct 21, 2021

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 21, 2021

nik9000 commented Oct 21, 2021

View reviewed changes

nik9000 added 6 commits October 21, 2021 12:39

Remove extra

c5f2f3f

Merge branch 'master' into aggregator_tests_round_trip

b677de4

Merge branch 'master' into aggregator_tests_round_trip

087f446

More slurping

a23a949

More

a0ea39e

More

826f7e0

nik9000 requested a review from not-napoleon October 25, 2021 13:26

Fixup multi terms

ec1fc5a

not-napoleon approved these changes Oct 25, 2021

View reviewed changes

nik9000 added 2 commits October 25, 2021 17:07

Merge branch 'master' into aggregator_tests_round_trip

157abb8

Fixup

48a80dc

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

elasticsearchmachine merged commit 2b5f6ce into elastic:master Oct 25, 2021

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

nik9000 added >bug v7.16.0 v8.1.0 and removed >test Issues or PRs that are addressing/adding tests labels Nov 10, 2021

imotov changed the title ~~Add extra round trip to aggs tests~~ Fix Unknown NamedWriteable error in InternalAggregation (Add extra round trip to aggs tests) Dec 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Unknown NamedWriteable error in InternalAggregation (Add extra round trip to aggs tests) #79638

Fix Unknown NamedWriteable error in InternalAggregation (Add extra round trip to aggs tests) #79638

nik9000 commented Oct 21, 2021 •

edited by imotov

Loading

elasticmachine commented Oct 21, 2021

nik9000 Oct 21, 2021

not-napoleon Oct 25, 2021

nik9000 Oct 25, 2021

nik9000 Oct 21, 2021

not-napoleon Oct 25, 2021

nik9000 Oct 21, 2021

nik9000 Oct 21, 2021

nik9000 Oct 21, 2021

nik9000 Oct 21, 2021

nik9000 Oct 21, 2021

not-napoleon Oct 25, 2021

nik9000 Oct 21, 2021

nik9000 Oct 21, 2021

nik9000 Oct 21, 2021

not-napoleon left a comment

not-napoleon Oct 25, 2021

not-napoleon Oct 25, 2021

not-napoleon Oct 25, 2021

not-napoleon Oct 25, 2021

nik9000 Oct 25, 2021

not-napoleon Oct 25, 2021

nik9000 Oct 25, 2021

nik9000 commented Nov 10, 2021

		/**
		* @return a random {@link DocValueFormat} that can be used in aggregations which

Fix Unknown NamedWriteable error in InternalAggregation (Add extra round trip to aggs tests) #79638

Fix Unknown NamedWriteable error in InternalAggregation (Add extra round trip to aggs tests) #79638

Conversation

nik9000 commented Oct 21, 2021 • edited by imotov Loading

elasticmachine commented Oct 21, 2021

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

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

not-napoleon 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

nik9000 commented Nov 10, 2021

nik9000 commented Oct 21, 2021 •

edited by imotov

Loading