Add doc_count field mapper by csoulios · Pull Request #58339 · elastic/elasticsearch

csoulios · 2020-06-18T16:17:10Z

Bucket aggregations compute bucket doc_count values by incrementing the doc_count by 1 for every document collected in the bucket.

When using summary fields (such as aggregate_metric_double) one field may represent more than one document. To provide this functionality we have implemented a new field mapper (named doc_count field mapper). This field is a positive integer representing the number of documents aggregated in a single summary field.

Bucket aggregations will check if a field of type doc_count exists in a document and will take this value into consideration when computing doc counts.

elasticmachine · 2020-07-09T16:24:46Z

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

elasticmachine · 2020-07-09T16:24:47Z

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

jtibshirani

Should we make this a metadata field _doc_count instead of a standard field type? It could be a nice fit, because it's more of an internal field rather than something we expect users to configure. Also we don't want multiple fields of type doc_count to be defined.

polyfractal

LGTM, although my mapper-skills are relatively poor so take it with a grain of salt. :)

Left a few comments about validation/tests but not critical (and might not be relevant). Exciting!

polyfractal · 2020-10-06T16:59:37Z

docs/reference/mapping/fields/doc-count-field.asciidoc

+
+[IMPORTANT]
+========
+* A `_doc_count` field can only store a single positive integer per document. Nested arrays are not allowed.


Random thought while reading the restrictions, is it possible to define _doc_count as an object? We should forbid that as well if it isn't already... but i suspect the current restrictions prevent it from being an object too.

There is an assertion that the input is a VALUE_NUMBER in the parseCreateField() method.

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

Line 113 in 7b7ca43

XContentParserUtils.ensureExpectedToken(XContentParser.Token.VALUE_NUMBER, parser.currentToken(), parser);

Is there anything else that should be added?

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

polyfractal · 2020-10-06T17:14:07Z

.../test/java/org/elasticsearch/search/aggregations/bucket/FieldBasedDocCountProviderTests.java

+import static java.util.Collections.singleton;
+
+
+public class FieldBasedDocCountProviderTests extends AggregatorTestCase {


Is it possible to filter or aggregate on the _doc_counts field itself...I'm guessing it's not possible? Should we add some tests to make sure that's handled politely if a user tries to search/agg it?

+1, it's common to have a test case like DocCountFieldTypeTests that checks query and field data construction works (or throws an error) as expected.

I have added test DocCountFieldTypeTests that tests range, term and exists queries.

Also, DocCountFieldType extends the MappedFieldType class that by default throws an exception when calling the fielddataBuilder(). Does this cover aggregations?

jtibshirani

It looks good to me overall, I left some minor comments.

docs/reference/mapping/fields.asciidoc

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

Cleaned up/simplified DocCountProvider class

csoulios · 2020-10-27T15:56:33Z

@elasticmachine run elasticsearch-ci/bwc
@elasticmachine run elasticsearch-ci/default-distro

jtibshirani

Thanks for the updates, I had one last comment.

jtibshirani · 2020-10-27T17:53:41Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/DocCountProvider.java

+        try {
+            docCountValues = DocValues.getNumeric(ctx.reader(), DocCountFieldMapper.NAME);
+        } catch (IOException e) {
+            docCountValues = null;


I wonder if we should be swallowing an IOException and treating it as if there was no doc count field. What case does this logic address?

Initially, I felt it was better to swallow the error and fallback to doc_count=1. Now, it feels more transparent to rethrow the exception.

I am not sure what can go wrong there and I don't have a strong opinion about this.

I agree we should propagate it, an IOException could indicate something serious and it's best not to silently continue on. Perhaps this method could just declare throws IOException.

jimczi

LGTM too.

csoulios · 2020-11-02T20:10:24Z

This PR is closed and opened as new PR (#64503) against master branch.

not-napoleon · 2020-11-02T19:37:06Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/DocCountProvider.java

+import java.io.IOException;
+
+/**
+ * An implementation of a doc_count provider that reads the value


Nit: Javadoc still reflects having an interface

not-napoleon · 2020-11-02T19:45:47Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/DocCountProvider.java

+        }
+    }
+
+    public void setLeafReaderContext(LeafReaderContext ctx) throws IOException {


Why isn't this just a constructor argument? Under what conditions would it be correct to use a DocCountProvider without setting the LeafReaderContext?

csoulios added 6 commits May 14, 2020 11:53

Initial version of doc_count field mapper

4b5fab3

added tests

cd515b3

Build fixes

655e112

Merge branch 'feature/aggregate-metrics' into doc_count-field-mapper

db13d83

Added tests for doc_count fieldmapper

191d793

doc count tests

5f81bee

csoulios added WIP :Analytics/Aggregations Aggregations v8.0.0 labels Jun 18, 2020

Merge branch 'feature/aggregate-metrics' into doc_count-field-mapper

dab8219

jtibshirani added the :Search Foundations/Mapping Index mappings, including merging and defining field types label Jun 25, 2020

csoulios added 7 commits June 30, 2020 13:30

Merge branch 'feature/aggregate-metrics' into doc_count-field-mapper

ecdc603

Resolve conflicts after merge from master

520ac9a

Added yaml test for doc_count field type

676ffc6

Merge branch 'feature/aggregate-metrics' into doc_count-field-mapper

7c7139c

Minor changes to test

d3b9c45

Fix issue with not-registering field mapper

c36ecac

Simplify terms agg test

4dca391

talevy added the Team:Deployment Management Meta label for Management Experience - Deployment Management team label Jul 6, 2020

$@polyfractal$ polyfractal mentioned this pull request Jul 8, 2020

[rollup] Why the documents of the rollup contain <field>.<agg_type>._count having the same values? #47876

Closed

csoulios marked this pull request as ready for review July 9, 2020 16:24

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

elasticmachine added the Team:Search Meta label for search team label Jul 9, 2020

csoulios removed the WIP label Jul 9, 2020

Add doc_count provider in the buckets aggregator

912d943

csoulios requested review from polyfractal and talevy July 9, 2020 16:31

jtibshirani reviewed Jul 9, 2020

View reviewed changes

Addressed review comments

b258653

$polyfractal$

polyfractal approved these changes Oct 6, 2020

View reviewed changes

jtibshirani reviewed Oct 6, 2020

View reviewed changes

jtibshirani mentioned this pull request Oct 12, 2020

Handle metadata fields in search 'fields' option #63569

Closed

csoulios added 6 commits October 19, 2020 16:10

Addressed reviewer comments

f5ed1df

Merge branch 'feature/aggregate-metrics' into doc_count-field-mapper

2fcdcf6

Added DocCountFieldTypeTests

4138d16

Merge branch 'feature/aggregate-metrics' into doc_count-field-mapper

5d38b7f

Fix errors after merge

654847e

Make composite agg respect _doc_count field

7b7ca43

Cleaned up/simplified DocCountProvider class

csoulios requested a review from jimczi October 27, 2020 15:49

csoulios requested a review from not-napoleon October 27, 2020 17:11

csoulios added 2 commits October 27, 2020 19:41

Merge branch 'feature/aggregate-metrics' into doc_count-field-mapper

ce44e87

Merge branch 'feature/aggregate-metrics' into doc_count-field-mapper

5621c44

jtibshirani reviewed Oct 27, 2020

View reviewed changes

DocCountProvider rethrows IOException instead of swallowing it

1d969a1

jtibshirani approved these changes Oct 29, 2020

View reviewed changes

jimczi approved these changes Nov 2, 2020

View reviewed changes

csoulios added 2 commits November 2, 2020 17:42

Set familyTypeName of _doc_count to integer

cb05034

Merge branch 'feature/aggregate-metrics' into doc_count-field-mapper

d7d80f4

csoulios mentioned this pull request Nov 2, 2020

Add doc_count field mapper #64503

Merged

csoulios closed this Nov 2, 2020

not-napoleon reviewed Nov 3, 2020

View reviewed changes

Mpdreamz mentioned this pull request Nov 16, 2020

7.10.1 Meta Ticket elastic/elasticsearch-net#5096

Closed

61 tasks

stevejgordon mentioned this pull request Dec 17, 2020

7.11.0 Meta Ticket elastic/elasticsearch-net#5198

Closed

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

csoulios deleted the doc_count-field-mapper branch October 14, 2022 13:41

		import static java.util.Collections.singleton;


		public class FieldBasedDocCountProviderTests extends AggregatorTestCase {

Conversation

csoulios commented Jun 18, 2020

Uh oh!

elasticmachine commented Jul 9, 2020

Uh oh!

elasticmachine commented Jul 9, 2020

Uh oh!

jtibshirani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polyfractal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtibshirani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

csoulios commented Oct 27, 2020

Uh oh!

jtibshirani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

csoulios commented Nov 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jtibshirani left a comment •

edited

Loading

$@polyfractal$ polyfractal left a comment

csoulios Oct 27, 2020 •

edited

Loading

csoulios Oct 27, 2020 •

edited

Loading

jtibshirani left a comment •

edited

Loading