Fix wrong error upper bound when performing incremental reductions by Hohol · Pull Request #43874 · elastic/elasticsearch

Hohol · 2019-07-02T13:01:16Z

When performing incremental reductions, 0 value of docCountError may mean that the error was not previously calculated, or that the error was indeed previously calculated and its value was 0. We end up rejecting true values set to 0 this way.
This may lead to wrong upper bound of error in result.
To fix it, this PR makes docCountError nullable. null values mean that error was not calculated yet.

Fixes #40005

elasticmachine · 2019-07-02T15:06:25Z

Pinging @elastic/es-analytics-geo

Hohol · 2019-07-05T10:49:41Z

@jpountz, @jimczi, can I get some reviewers, please?

polyfractal · 2019-07-08T16:27:13Z

Apologies for the delay on a review, lots of folks have been in/out due to summer holidays :)

I'll take a look at it early this week, and I've also requested a review from @javanna when he gets back from holiday (also early this week). Thanks for the PR @Hohol!

Hohol · 2019-11-15T12:03:12Z

I've updated the PR. Can we finish this?
Is the issue still relevant at all?

Hohol · 2019-11-15T12:03:52Z

Oops, looks like diff is unreadable now.

Hohol · 2019-11-15T12:10:58Z

Rebased.

Hohol · 2019-11-21T11:26:35Z

@polyfractal, @javanna, @colings86, can someone answer, please?

nik9000

I think this is right. I'm really sorry we lost track of this one. It seems like a good change. I left a small question, but I think its right.

@imotov thought I knew this code fairly well. I'm not 100% confident in my ability to reason about it all in my head but with the test change and his confidence I'm happy with the change.

nik9000 · 2021-07-01T19:20:47Z

...er/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/InternalMappedTerms.java

        super(in);
-        docCountError = in.readZLong();
+        if (in.getVersion().onOrAfter(Version.V_8_0_0)) { // todo fix after backport
+            docCountError = in.readOptionalLong();


Should this stay a zlong?

@nik9000 why was it a zlong in the first place? Can errors be negative?

nik9000 · 2021-07-01T23:45:33Z

Good question! Maybe it should be vlong? I'm not sure how it got that way in the first place.

…

On Thu, Jul 1, 2021, 15:49 Igor Motov ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/InternalMappedTerms.java <#43874 (comment)> : > @@ -51,7 +52,14 @@ protected InternalMappedTerms(String name, BucketOrder reduceOrder, BucketOrder */ protected InternalMappedTerms(StreamInput in, Bucket.Reader<B> bucketReader) throws IOException { super(in); - docCountError = in.readZLong(); + if (in.getVersion().onOrAfter(Version.V_8_0_0)) { // todo fix after backport + docCountError = in.readOptionalLong(); @nik9000 <https://github.com/nik9000> why was it a zlong in the first place? Can errors be negative? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#43874 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABUXIQWY26EEDI4XDXEWO3TVTBFJANCNFSM4H43R3RA> .

imotov · 2021-07-06T15:44:14Z

@elasticmachine update branch

imotov · 2021-07-06T15:44:44Z

@elasticmachine test this please

not-napoleon

I've read this twice now, and I think it's okay. Very tricky.

not-napoleon · 2021-07-20T19:56:35Z

...ernalClusterTest/java/org/elasticsearch/search/aggregations/bucket/TermsDocCountErrorIT.java

        assertThat(bucket.getDocCountError(), equalTo(29L));
    }

+    public void testIncrementalReductionBug() {


I'm nitpicking, but it'd be good to explain exactly what this is testing, or at least link to the issue. In a year, we'll have no idea what it's supposed to do.

imotov · 2021-07-21T16:39:54Z

@elasticmachine update branch

imotov · 2021-07-21T16:47:46Z

@elasticmachine test this please

imotov · 2021-07-21T18:31:35Z

@elasticmachine run elasticsearch-ci/part-1

…lastic#43874) When performing incremental reductions, 0 value of docCountError may mean that the error was not previously calculated, or that the error was indeed previously calculated and its value was 0. We end up rejecting true values set to 0 this way. This may lead to wrong upper bound of error in result. To fix it, this PR makes docCountError nullable. null values mean that error was not calculated yet. Fixes elastic#40005 Co-authored-by: Igor Motov <igor@motovs.org> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Fix docCountError calculation in case of multiple reduces. It fixes 2 mistakes in elastic#43874. The first error was introduced in the original PR, where unknown doc count errors were initialized equal to 0, the second was introduced during in order to fix the first one by ignoring these 0s, which essentially disabled the original fix. Fixes elastic#75667

Fix docCountError calculation in case of multiple reduces. It fixes 2 mistakes in #43874. The first error was introduced in the original PR, where unknown doc count errors were initialized equal to 0, the second was introduced during in order to fix the first one by ignoring these 0s, which essentially disabled the original fix. Fixes #75667

…lastic#43874) When performing incremental reductions, 0 value of docCountError may mean that the error was not previously calculated, or that the error was indeed previously calculated and its value was 0. We end up rejecting true values set to 0 this way. This may lead to wrong upper bound of error in result. To fix it, this PR makes docCountError nullable. null values mean that error was not calculated yet. Fixes elastic#40005 Co-authored-by: Igor Motov <igor@motovs.org> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Fix docCountError calculation in case of multiple reduces. It fixes 2 mistakes in elastic#43874. The first error was introduced in the original PR, where unknown doc count errors were initialized equal to 0, the second was introduced during in order to fix the first one by ignoring these 0s, which essentially disabled the original fix. Fixes elastic#75667

…ons (#43874) (#76475) When performing incremental reductions, 0 value of docCountError may mean that the error was not previously calculated, or that the error was indeed previously calculated and its value was 0. We end up rejecting true values set to 0 this way. This may lead to wrong upper bound of error in result. To fix it, this PR makes docCountError nullable. null values mean that error was not calculated yet. Fixes #40005, #75667 Co-authored-by: Nikita Glashenko <nikita.glashenko@gmail.com>

javanna · 2022-04-01T13:57:24Z

I was expecting this to address an issue I found in CCSDuelIT, but that does not seem to be the case. See #85538 . We still have an awaitsfix and a failure when we try and remove it. Could you help with this?

jimczi added :Analytics/Aggregations Aggregations >bug labels Jul 2, 2019

jimczi added v7.3.0 v8.0.0 labels Jul 2, 2019

jpountz added v7.4.0 and removed v7.3.0 labels Jul 3, 2019

Hohol marked this pull request as ready for review July 3, 2019 16:30

Hohol mentioned this pull request Jul 3, 2019

Error upper bound may be wrong when performing incremental reductions #40005

Closed

$@polyfractal$ polyfractal requested review from javanna and polyfractal July 8, 2019 16:26

colings86 added v7.5.0 and removed v7.4.0 labels Aug 30, 2019

jimczi added v7.6.0 and removed v7.5.0 labels Nov 12, 2019

Make error upper bound nullable

5c6d3c6

$@polyfractal$ polyfractal added v7.7.0 and removed v7.6.0 labels Jan 15, 2020

bpintea added v7.8.0 and removed v7.7.0 labels Mar 25, 2020

rjernst added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 4, 2020

pugnascotia added v7.8.1 v7.9.0 and removed v7.8.0 labels May 6, 2020

nik9000 reviewed Jul 1, 2021

View reviewed changes

Merge branch 'master' into nullable-doc-count-error

1000f52

not-napoleon self-requested a review July 13, 2021 17:56

not-napoleon approved these changes Jul 20, 2021

View reviewed changes

elasticmachine and others added 2 commits July 21, 2021 18:39

Merge branch 'master' into nullable-doc-count-error

1613eac

Clarify the purpose of the incremental reduction test

d953d5f

imotov merged commit 1db17ad into elastic:master Jul 22, 2021

imotov added the backport pending label Jul 22, 2021

Hohol deleted the nullable-doc-count-error branch July 23, 2021 08:21

jakelandis added v8.0.0-alpha1 v8.0.0 and removed v8.0.0 v8.0.0-alpha1 labels Jul 26, 2021

imotov mentioned this pull request Aug 11, 2021

Fix docCountError calculation for multiple reduces #76391

Merged

joegallo removed the backport pending label Sep 22, 2021

Conversation

Hohol commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 2, 2019

Uh oh!

Hohol commented Jul 5, 2019

Uh oh!

polyfractal commented Jul 8, 2019

Uh oh!

Hohol commented Nov 15, 2019

Uh oh!

Hohol commented Nov 15, 2019

Uh oh!

Hohol commented Nov 15, 2019

Uh oh!

Hohol commented Nov 21, 2019

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

imotov Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jul 1, 2021 via email

Uh oh!

imotov commented Jul 6, 2021

Uh oh!

imotov commented Jul 6, 2021

Uh oh!

not-napoleon left a comment

Choose a reason for hiding this comment

Uh oh!

not-napoleon Jul 20, 2021

Choose a reason for hiding this comment

Uh oh!

imotov commented Jul 21, 2021

Uh oh!

imotov commented Jul 21, 2021

Uh oh!

imotov commented Jul 21, 2021

Uh oh!

javanna commented Apr 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

18 participants

Hohol commented Jul 2, 2019 •

edited

Loading