Support _first and _last parameter for missing bucket ordering in composite aggregation by penghuo · Pull Request #1942 · opensearch-project/OpenSearch

penghuo · 2022-01-19T20:00:08Z

Description

Support _first and _last parameter for missing bucket ordering in composite aggregation.
If missing_order is "_first" or "_last", regardless order, missing_bucket is at first or last respectively. for example, the following query will sort product asc and null bucket will be placed at last.

{
  "size": 0,
  "aggs": {
    "my_buckets": {
      "composite": {
        "sources": [
          {
            "product_name": {
              "terms": {
                "field": "product",
                "missing_bucket": true,
                "missing_order": "last",
                "order": "asc"
              }
            }
          }
        ]
      }
    }
  }
}

Issues Resolved

#1899

Check List

[x ] New functionality includes testing.
- [x ] All tests pass
New functionality has been documented.
- [x ] New functionality has javadoc added
[ x] 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.

By default, if order is asc, missing_bucket at first, if order is desc, missing_bucket at last. If missing_order is "_first" or "_last", regardless order, missing_bucket is at first or last respectively. Signed-off-by: Peng Huo <penghuo@gmail.com>

opensearch-ci-bot · 2022-01-19T20:02:04Z

Can one of the admins verify this patch?

opensearch-ci-bot · 2022-01-19T20:49:47Z

✅ Gradle Check success 134f068
Log 1990

Reports 1990

Signed-off-by: Peng Huo <penghuo@gmail.com>

opensearch-ci-bot · 2022-01-20T02:21:45Z

✅ Gradle Check success e9aed4d
Log 1998

Reports 1998

saratvemulapalli

Thanks for this. It looks good to me as far as I understand.
Also we should add documentation for this feature, on OpenSearch its usually via Java docs.

I would like @nknize / @andrross to review for another pair of eyes.

saratvemulapalli · 2022-01-21T20:23:33Z

...n/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java

+        if (in.getVersion().onOrAfter(Version.V_2_0_0)) {
+            this.missingOrder = MissingOrder.readFromStream(in);
+        }


Do you plan to leave this feature only for 2.0.0?
I dont see any harm to include this for 1.3.0.

1.3.0 also works, there is no blocker.
totally agree, doc is necessary. i add javadoc on setter method. any suggestion for others?

If you'd like this for 1.3.0 (I'm good w/ that) let's make the version change in a separate backportable PR.

I prefer a separate backportable PR for 1.3.0.

nknize

Overall LGTM! Just a few nit picky changes.

...er/src/main/java/org/opensearch/search/aggregations/bucket/composite/BinaryValuesSource.java

nknize · 2022-01-26T17:07:00Z

...n/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java

+        if (in.getVersion().onOrAfter(Version.V_2_0_0)) {
+            this.missingOrder = MissingOrder.readFromStream(in);
+        }


If you'd like this for 1.3.0 (I'm good w/ that) let's make the version change in a separate backportable PR.

...n/java/org/opensearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java

...main/java/org/opensearch/search/aggregations/bucket/composite/GlobalOrdinalValuesSource.java

server/src/main/java/org/opensearch/search/aggregations/bucket/composite/LongValuesSource.java

.../test/java/org/opensearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java

Signed-off-by: Peng Huo <penghuo@gmail.com>

penghuo · 2022-01-26T20:20:32Z

@nknize Thanks for the feedback, good point. I pushed a fix and resolve comments which I already addressed, but I am leaving 1 comment unresolved and want to double confirm with you.

opensearch-ci-bot · 2022-01-26T21:03:51Z

✅ Gradle Check success 04517f5
Log 2065

Reports 2065

nknize · 2022-01-26T22:35:16Z

I prefer a separate backportable PR for 1.3.0.

I'm good w/ that!

nknize

LGTM! Thx for the quick turnaround!

…r parameter (opensearch-project#1942) InternalBucket should use MissingOrder to decide null values's order. Signed-off-by: Peng Huo <penghuo@gmail.com>

…r parameter (#1942) (#2005) Fix composite aggregation failed test cases introduce by missing_order parameter by using MissingOrder to decide null values's order in InternalBucket. Signed-off-by: Peng Huo <penghuo@gmail.com>

…site aggregation (opensearch-project#1942) Support for "first" and "last" parameters for missing bucket ordering in composite aggregation. By default, if order is asc, missing_bucket at first, if order is desc, missing_bucket at last. If missing_order is "first" or "last", regardless order, missing_bucket is at first or last respectively. Signed-off-by: Peng Huo <penghuo@gmail.com>

…site aggregation (#1942) (#2049) Support for "first" and "last" parameters for missing bucket ordering in composite aggregation. By default, if order is asc, missing_bucket at first, if order is desc, missing_bucket at last. If missing_order is "first" or "last", regardless order, missing_bucket is at first or last respectively. Signed-off-by: Peng Huo <penghuo@gmail.com>

…in composite aggregation (opensearch-project#1942) (opensearch-project#2049)" This reverts commit 5b27136. Signed-off-by: Andrew Ross <andrross@amazon.com>

penghuo marked this pull request as ready for review January 19, 2022 20:55

penghuo requested a review from a team as a code owner January 19, 2022 20:55

throw exception if missing_order required missing_bucket is true

e9aed4d

Signed-off-by: Peng Huo <penghuo@gmail.com>

saratvemulapalli approved these changes Jan 21, 2022

View reviewed changes

nknize suggested changes Jan 26, 2022

View reviewed changes

address comments

04517f5

Signed-off-by: Peng Huo <penghuo@gmail.com>

nknize self-requested a review January 26, 2022 22:35

nknize approved these changes Jan 26, 2022

View reviewed changes

nknize merged commit 7a9314a into opensearch-project:main Jan 26, 2022

nknize mentioned this pull request Jan 27, 2022

[CI] test failure in o.o.search.aggregations.bucket.composite.CompositeAggregatorTests.testWithKeywordLongAndMissingBucket #1998

Closed

penghuo mentioned this pull request Jan 28, 2022

Fix composite aggregation failed test cases introduce by missing_order parameter (#1942) #2005

Merged

5 tasks

penghuo mentioned this pull request Feb 1, 2022

Support _first and _last parameter for missing bucket ordering in composite aggregation #1941

Closed

1 task

dblock mentioned this pull request Feb 2, 2022

Support _first and _last parameter for missing bucket ordering in composite aggregation #1899

Closed

penghuo mentioned this pull request Feb 3, 2022

[Backport 1.x] Support first and last parameter for missing bucket ordering in composite aggregation #2049

Merged

4 tasks

andrross mentioned this pull request Feb 16, 2022

Revert "Support first and last parameter for missing bucket ordering … #2145

Closed

5 tasks

Conversation

penghuo commented Jan 19, 2022

Description

Issues Resolved

Check List

Uh oh!

opensearch-ci-bot commented Jan 19, 2022

Uh oh!

opensearch-ci-bot commented Jan 19, 2022

Uh oh!

opensearch-ci-bot commented Jan 20, 2022

Uh oh!

saratvemulapalli 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!

saratvemulapalli Jan 21, 2022

Choose a reason for hiding this comment

Uh oh!

penghuo Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

nknize Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

penghuo Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nknize Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

penghuo commented Jan 26, 2022

Uh oh!

opensearch-ci-bot commented Jan 26, 2022

Uh oh!

nknize commented Jan 26, 2022

Uh oh!

nknize left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

saratvemulapalli left a comment •

edited

Loading