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

Update the example of the multiple terms aggregation by muliple orders. #7039

Conversation

Zhikai-VM
Copy link
Contributor

@Zhikai-VM Zhikai-VM commented Apr 28, 2024

Description

Update the example of the multiple terms aggregation.

Issues Resolved

Closes #7037

Checklist

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

@ankyhe
Copy link

ankyhe commented Apr 28, 2024

I suggest we don't remove the original example, but add a new one. Because if it needs only 1 order, it could use original one. If we change document like this way, it could confuse the old order style is not supported. But it is not true, it can support both one order with {...} or multiple order with [...].

@Zhikai-VM
Copy link
Contributor Author

I suggest we don't remove the original example, but add a new one. Because if it needs only 1 order, it could use original one. If we change document like this way, it could confuse the old order style is not supported. But it is not true, it can support both one order with {...} or multiple order with [...].

I got your point, but the current document is not correct. I am trying to fix it to make it accurate.

@hdhalter
Copy link
Contributor

@bowenlan-amzn - What are your thoughts on this?

@hdhalter hdhalter added the 3 - Tech review PR: Tech review in progress label Apr 29, 2024
@Zhikai-VM
Copy link
Contributor Author

@bowenlan-amzn - What are your thoughts on this?

Please refer to the update in PR: opensearch-project/OpenSearch#13400

@hdhalter
Copy link
Contributor

hdhalter commented May 2, 2024

Hi @dblock - Can you please comment with your thoughts? Thanks.

@dblock
Copy link
Member

dblock commented May 6, 2024

The current example is incorrect, if I understand correctly it has to be an array of orders, even with 1 order. There's no point in having an array of 1 and then another example with an array of 2, is there?

@hdhalter hdhalter added the backport 2.13 PR: Backport label for 2.13 label May 6, 2024
@Zhikai-VM
Copy link
Contributor Author

The current example is incorrect, if I understand correctly it has to be an array of orders, even with 1 order. There's no point in having an array of 1 and then another example with an array of 2, is there?

As far as I know, there is no example of array in the offical doc.

@ankyhe
Copy link

ankyhe commented May 7, 2024

The current example is incorrect, if I understand correctly it has to be an array of orders, even with 1 order. There's no point in having an array of 1 and then another example with an array of 2, is there?

@dblock As current official document, the multi-term example is as below:

GET sample-index100/_search
{
  "size": 0, 
  "aggs": {
    "hot": {
      "multi_terms": {
        "terms": [{
          "field": "region" 
        },{
          "field": "host" 
        }],
        "order": {"max-cpu": "desc"}
      },
      "aggs": {
        "max-cpu": { "max": { "field": "cpu" } }
      }      
    }
  }
}

@Zhikai-VM wants to revise it as:

GET sample-index100/_search
{
  "size": 0, 
  "aggs": {
    "hot": {
      "multi_terms": {
        "terms": [{
          "field": "region" 
        },{
          "field": "host" 
        }],
        "order": [ {"max-cpu": "desc"}, {"max-mem": "asc"}]
      },
      "aggs": {
        "max-cpu": { "max": { "field": "cpu" } },
        "max-mem": { "max": { "field": "mem"}}
      }      
    }
  }
}

to keep the official document accordance with this PR: opensearch-project/OpenSearch#13400

@Zhikai-VM
Copy link
Contributor Author

Hi @dblock , what do you think about this? I prefer to use an array for the 'order' field. We can pass one order or multiple orders in this field.

@Zhikai-VM
Copy link
Contributor Author

Hi @dblock and @bowenlan-amzn, any thoughts on this PR?

@dblock
Copy link
Member

dblock commented May 14, 2024

This looks good to me, can we merge @hdhalter ?

@hdhalter hdhalter added 4 - Doc review PR: Doc review in progress and removed 3 - Tech review PR: Tech review in progress labels May 14, 2024
@Naarcha-AWS Naarcha-AWS merged commit 927ca93 into opensearch-project:main May 14, 2024
7 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 14, 2024
…s. (#7039)

Signed-off-by: Zhikai Chen <[email protected]>
(cherry picked from commit 927ca93)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@Zhikai-VM
Copy link
Contributor Author

Thanks @dblock @bowenlan-amzn!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Doc review PR: Doc review in progress backport 2.13 PR: Backport label for 2.13
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC] Update the document for multiple terms aggregation
5 participants