Make global ords terms simpler to understand #57241

nik9000 · 2020-05-27T18:34:00Z

When the terms enum operates on non-numeric data it can collect it via
global ordinals. It actually has two separate collection strategies for this,
one "dense" and one "remapping". Each of those strategies has two
"iteration" strategies that it uses to build buckets, depending on
whether or not we need buckets with 0 docs in them. Previously this
was done with several null checks and never really explained. This
change replaces those checks with two CollectionStrategy classes which
have good stuff like Javadocs.

It also adds the name of the strategy used to the debugging information.

Related to #56487

When the `terms` enum operates on non-numeric data it can collect it via global ordinals. It actually has two separate collection strategies for, one "dense" and one "remapping". Each of *those* strategies has two "iteration" strategies that it uses to build buckets, depending on whether or not we need buckets with `0` docs in them. Previously this was done with several `null` checks and never really explained. This change replaces those checks with two `CollectionStrategy` classes which have good stuff like documentation.

nik9000 · 2020-05-27T18:34:26Z

Ah! I should add a profiler test for the collection_strategy debug information.

elasticmachine · 2020-05-28T13:19:35Z

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

not-napoleon

This looks good, definitely more clear than what we had before.

not-napoleon · 2020-05-28T14:23:09Z

.../org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java

+         */
+        abstract void globalOrdsReady(SortedSetDocValues globalOrds);
+        /**
+         * Collect a global ordinal.


I presume this is what gets called by the collector, but I think it'd help to make this javadoc a little more explicit.

not-napoleon · 2020-05-28T14:55:01Z

.../org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java

-    boolean remapGlobalOrds() {
-        return bucketOrds != null;
+        this.acceptedGlobalOrdinals = includeExclude == null ? l -> true : includeExclude.acceptedGlobalOrdinals(values)::get;
+        this.collectionStrategy = remapGlobalOrds ? new RemapGlobalOrds() : new DenseGlobalOrds();


Is there a reason not to just pass the collection strategy in directly?

Hmmmm - I did think about it but we'd have to do that same "pass a builder" kind of thing because it is a non-static inner class.

not-napoleon · 2020-05-28T15:04:05Z

.../org/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java

    }
+
+    /**
+     * Strategy for collecting global ordinals.


I think your comment from the PR description that we have two collection strategies and each has two iteration strategies would fit well in the javadoc for this class. Or at the very least, a note on forEach that it should account for both iteration cases.

nik9000 · 2020-05-28T17:58:32Z

This looks good, definitely more clear than what we had before.

It took quite a bit of staring for me to figure out what we had before!

nik9000 · 2020-05-28T19:30:06Z

Thanks @not-napoleon! I've added some javadoc and merged. If you can think of a way to pass the strategy in that you'd prefer please ping me!

When the `terms` enum operates on non-numeric data it can collect it via global ordinals. It actually has two separate collection strategies for, one "dense" and one "remapping". Each of *those* strategies has two "iteration" strategies that it uses to build buckets, depending on whether or not we need buckets with `0` docs in them. Previously this was done with several `null` checks and never really explained. This change replaces those checks with two `CollectionStrategy` classes which have good stuff like documentation.

I accidentally didn't put the customary "skip the last version" on #57241 and the PR tests didn't catch it. This adds it.

…7311) When the `terms` enum operates on non-numeric data it can collect it via global ordinals. It actually has two separate collection strategies for, one "dense" and one "remapping". Each of *those* strategies has two "iteration" strategies that it uses to build buckets, depending on whether or not we need buckets with `0` docs in them. Previously this was done with several `null` checks and never really explained. This change replaces those checks with two `CollectionStrategy` classes which have good stuff like documentation.

When the `terms` agg runs against strings and uses global ordinals it has an optimization when it collects segments that only ever have a single value for the particular string. This is *very* common. But I broke it in elastic#57241. This fixes that optimization and adds `debug` information that you can use to see how often we collect segments of each type. And adds a test to make sure that I don't break the optimization again. We also had a specialiation for when there isn't a filter on the terms to aggregate. I had removed that specialization in elastic#57241 which resulted in some slow down as well. This adds it back but in a more clear way. And, hopefully, a way that is marginally faster when there *is* a filter. Closes elastic#57407

When the `terms` agg runs against strings and uses global ordinals it has an optimization when it collects segments that only ever have a single value for the particular string. This is *very* common. But I broke it in #57241. This fixes that optimization and adds `debug` information that you can use to see how often we collect segments of each type. And adds a test to make sure that I don't break the optimization again. We also had a specialiation for when there isn't a filter on the terms to aggregate. I had removed that specialization in #57241 which resulted in some slow down as well. This adds it back but in a more clear way. And, hopefully, a way that is marginally faster when there *is* a filter. Closes #57407

When the `terms` agg runs against strings and uses global ordinals it has an optimization when it collects segments that only ever have a single value for the particular string. This is *very* common. But I broke it in elastic#57241. This fixes that optimization and adds `debug` information that you can use to see how often we collect segments of each type. And adds a test to make sure that I don't break the optimization again. We also had a specialiation for when there isn't a filter on the terms to aggregate. I had removed that specialization in elastic#57241 which resulted in some slow down as well. This adds it back but in a more clear way. And, hopefully, a way that is marginally faster when there *is* a filter. Closes elastic#57407

When the `terms` agg runs against strings and uses global ordinals it has an optimization when it collects segments that only ever have a single value for the particular string. This is *very* common. But I broke it in #57241. This fixes that optimization and adds `debug` information that you can use to see how often we collect segments of each type. And adds a test to make sure that I don't break the optimization again. We also had a specialiation for when there isn't a filter on the terms to aggregate. I had removed that specialization in #57241 which resulted in some slow down as well. This adds it back but in a more clear way. And, hopefully, a way that is marginally faster when there *is* a filter. Closes #57407

nik9000 added 3 commits May 27, 2020 16:43

fix

f15258e

Merge branch 'master' into global_ords_terms_collection_strategy

8e42d86

Be good now!

11f0bea

nik9000 marked this pull request as ready for review May 28, 2020 13:19

nik9000 requested a review from not-napoleon May 28, 2020 13:19

nik9000 added :Analytics/Aggregations Aggregations v7.9.0 v8.0.0 labels May 28, 2020

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

not-napoleon approved these changes May 28, 2020

View reviewed changes

nik9000 added 2 commits May 28, 2020 14:06

Javadoc

4a8e3aa

Fix bug

b2eb394

nik9000 merged commit 974d236 into elastic:master May 28, 2020

nik9000 added the backport pending label May 28, 2020

nik9000 added a commit that referenced this pull request May 28, 2020

Update skip before backport

29e9e79

I accidentally didn't put the customary "skip the last version" on #57241 and the PR tests didn't catch it. This adds it.

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request May 28, 2020

Update skip after backport of elastic#57241

1529e53

nik9000 added a commit that referenced this pull request May 29, 2020

Update skip after backport of #57241 (#57316)

d0a253d

nik9000 removed the backport pending label May 29, 2020

dliappis mentioned this pull request Jun 1, 2020

Regression in aggregation latency after #57241 #57407

Closed

nik9000 mentioned this pull request Jun 1, 2020

Fix an optimization in terms agg #57438

Merged

nik9000 mentioned this pull request Jun 2, 2020

Fix an optimization in terms agg (backport #57438) #57547

Merged

pugnascotia added the >refactoring label Jul 16, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make global ords terms simpler to understand #57241

Make global ords terms simpler to understand #57241

Uh oh!

nik9000 commented May 27, 2020 •

edited

Loading

Uh oh!

nik9000 commented May 27, 2020

Uh oh!

elasticmachine commented May 28, 2020

Uh oh!

not-napoleon left a comment

Uh oh!

not-napoleon May 28, 2020

Uh oh!

nik9000 May 28, 2020

Uh oh!

not-napoleon May 28, 2020

Uh oh!

nik9000 May 28, 2020

Uh oh!

not-napoleon May 28, 2020

Uh oh!

nik9000 May 28, 2020

Uh oh!

nik9000 commented May 28, 2020

Uh oh!

nik9000 commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Make global ords terms simpler to understand #57241

Make global ords terms simpler to understand #57241

Uh oh!

Conversation

nik9000 commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nik9000 commented May 27, 2020

Uh oh!

elasticmachine commented May 28, 2020

Uh oh!

not-napoleon left a comment

Choose a reason for hiding this comment

Uh oh!

not-napoleon May 28, 2020

Choose a reason for hiding this comment

Uh oh!

nik9000 May 28, 2020

Choose a reason for hiding this comment

Uh oh!

not-napoleon May 28, 2020

Choose a reason for hiding this comment

Uh oh!

nik9000 May 28, 2020

Choose a reason for hiding this comment

Uh oh!

not-napoleon May 28, 2020

Choose a reason for hiding this comment

Uh oh!

nik9000 May 28, 2020

Choose a reason for hiding this comment

Uh oh!

nik9000 commented May 28, 2020

Uh oh!

nik9000 commented May 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nik9000 commented May 27, 2020 •

edited

Loading