Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_doc_count elasticsearch PR #3985

Merged
merged 11 commits into from
Jul 28, 2022
Merged

Conversation

petardz
Copy link
Contributor

@petardz petardz commented Jul 22, 2022

Description

Merged elasticsearch _doc_count PR

Issues Resolved

3712

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

petardz added 3 commits July 22, 2022 16:35
Signed-off-by: Petar Dzepina <[email protected]>
Signed-off-by: Petar Dzepina <[email protected]>
@petardz petardz requested review from a team and reta as code owners July 22, 2022 16:49
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@praveensameneni
Copy link
Member

praveensameneni commented Jul 22, 2022

@nknize , @saratvemulapalli can you please approve workflows and also review the PR. This helps in one of our Rollup enhancement issues

Signed-off-by: Petar Dzepina <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Petar Dzepina <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Petar Dzepina <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Petar Dzepina <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2022

Codecov Report

Merging #3985 (5907dbc) into main (ccf1d15) will increase coverage by 0.22%.
The diff coverage is 86.66%.

@@             Coverage Diff              @@
##               main    #3985      +/-   ##
============================================
+ Coverage     70.42%   70.65%   +0.22%     
- Complexity    56659    56817     +158     
============================================
  Files          4573     4575       +2     
  Lines        273348   273401      +53     
  Branches      40088    40092       +4     
============================================
+ Hits         192512   193160     +648     
+ Misses        64586    63998     -588     
+ Partials      16250    16243       -7     
Impacted Files Coverage Δ
...ns/bucket/adjacency/AdjacencyMatrixAggregator.java 19.78% <0.00%> (ø)
...g/opensearch/index/mapper/DocCountFieldMapper.java 79.41% <79.41%> (ø)
...h/search/aggregations/bucket/DocCountProvider.java 83.33% <83.33%> (ø)
...ain/java/org/opensearch/indices/IndicesModule.java 98.19% <100.00%> (+0.01%) ⬆️
...opensearch/search/aggregations/AggregatorBase.java 81.69% <100.00%> (ø)
.../search/aggregations/bucket/BucketsAggregator.java 90.64% <100.00%> (+0.34%) ⬆️
...egations/bucket/composite/CompositeAggregator.java 88.26% <100.00%> (+0.52%) ⬆️
...ucket/composite/CompositeValuesCollectorQueue.java 92.07% <100.00%> (ø)
...regations/bucket/composite/SortedDocsProducer.java 87.09% <100.00%> (+1.38%) ⬆️
...h/aggregations/bucket/nested/NestedAggregator.java 86.84% <100.00%> (+0.17%) ⬆️
... and 534 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@saratvemulapalli
Copy link
Member

@petardz these changes look similar to the PR in Elasticsearch.
Whats the under lying license and why was this not part of the fork ?

I didnt read up the changes, but I'll take a look.

@praveensameneni
Copy link
Member

@petardz these changes look similar to the PR in Elasticsearch. Whats the under lying license and why was this not part of the fork ?

I didnt read up the changes, but I'll take a look.

@saratvemulapalli , the PR was originally created before the fork and has the Apache-2 license.

@dblock
Copy link
Member

dblock commented Jul 27, 2022

thanks @praveensameneni, but I think @saratvemulapalli is asking the right questions - why wasn't this code included in the fork if it was PRed and merged before the fork?

Either way, @petardz can you please visibly confirm that your contributed code in this PR was written without looking/copying any non-APLv2 code?

@petardz
Copy link
Contributor Author

petardz commented Jul 27, 2022

@dblock Yes, I've just re-checked. The only exception is 380_doc_count_field.yml rest-api test which didn't have any license in file.(none of these .yml files do)

@nknize
Copy link
Collaborator

nknize commented Jul 28, 2022

why wasn't this code included in the fork if it was PRed and merged before the fork?

It was merged to master, not 7.x.

Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source code in this PR is taken from Elastic commit 330d2b74f93c71707e4192ad07c9a6a47d451575. Since the default license for that commit is ALv2 any files w/o license headers is covered under ALv2.

We do, however, need to update all of the headers in this PR to match the other forked source files so that attribution is correct:

/*
 * SPDX-License-Identifier: Apache-2.0
 *
 * The OpenSearch Contributors require contributions made to
 * this file be licensed under the Apache-2.0 license or a
 * compatible open source license.
 */

/*
 * Licensed to Elasticsearch under one or more contributor
 * license agreements. See the NOTICE file distributed with
 * this work for additional information regarding copyright
 * ownership. Elasticsearch licenses this file to you under
 * the Apache License, Version 2.0 (the "License"); you may
 * not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *    http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing,
 * software distributed under the License is distributed on an
 * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 * KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations
 * under the License.
 */

/*
 * Modifications Copyright OpenSearch Contributors. See
 * GitHub history for details.
 */

petardz added 2 commits July 28, 2022 18:29
Signed-off-by: Petar Dzepina <[email protected]>
Signed-off-by: Petar Dzepina <[email protected]>
@petardz
Copy link
Contributor Author

petardz commented Jul 28, 2022

@nknize done

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx for the quick turnaround. Looks like one file is still missing the OpenSearch license provisions.

@@ -0,0 +1,49 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is the last file missing the OpenSearch license provisions

Copy link
Contributor Author

@petardz petardz Jul 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah... sorry. Should be good now.

Signed-off-by: Petar Dzepina <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nknize nknize merged commit fb7d81a into opensearch-project:main Jul 28, 2022
@nknize nknize added feature New feature or request Indexing & Search v3.0.0 Issues and PRs related to version 3.0.0 backport 2.x Backport to 2.x branch labels Jul 28, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 28, 2022
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 this commit implements 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 check if a field of type doc_count exists in a
document and take this value into consideration when computing doc
counts.

Note: This originated from upstream PR 64503.

Signed-off-by: Petar Dzepina <[email protected]>
(cherry picked from commit fb7d81a)
dblock pushed a commit that referenced this pull request Aug 1, 2022
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 this commit implements 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 check if a field of type doc_count exists in a
document and take this value into consideration when computing doc
counts.

Note: This originated from upstream PR 64503.

Signed-off-by: Petar Dzepina <[email protected]>
(cherry picked from commit fb7d81a)

Co-authored-by: Petar Dzepina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch feature New feature or request Indexing & Search v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants