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

[BUG] The comments did not match the codes in MultiTermsAggregationBuilder #13375

Closed
Zhikai-VM opened this issue Apr 25, 2024 · 4 comments · Fixed by #13400
Closed

[BUG] The comments did not match the codes in MultiTermsAggregationBuilder #13375

Zhikai-VM opened this issue Apr 25, 2024 · 4 comments · Fixed by #13400
Labels

Comments

@Zhikai-VM
Copy link
Contributor

Zhikai-VM commented Apr 25, 2024

Describe the bug

In MultiTermsAggregationBuilder.java, the comments about multiple terms aggregation did not match codes:

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

The comment shows we can set up one order. But from the codes, it also supports an array like:
"order": [ {"max-cpu": "desc"} , {"max-memory": "desc"} ]

Here are the codes to parser this field:

PARSER.declareObjectArray(MultiTermsAggregationBuilder::order, (p, c) -> InternalOrder.Parser.parseOrderParam(p), ORDER_FIELD);

I also go through the codes and had some local tests. I think multiple orders do work. If so, could you please update the comment and the official document about this?

Related component

Search:Aggregations

To Reproduce

Compare the comments and codes.

Expected behavior

If multiple orders are supported, we should update the comments and the official document.

Additional Details

No response

@dblock
Copy link
Member

dblock commented Apr 25, 2024

Thanks @Zhikai-VM, looks like a real issue. Do you think you could help with the updates? Make PRs?

@Zhikai-VM
Copy link
Contributor Author

Thanks @Zhikai-VM, looks like a real issue. Do you think you could help with the updates? Make PRs?

Hi @dblock, yes, I could update the comment in a PR, but could you please help to double-confirm the case?

{
     "size": 0,
     "aggs": {
       "hot": {
         "multi_terms": {
           "terms": [{
             "field": "region"
           },{
             "field": "host"
           }],
           "order": [{"max-cpu": "desc"},  {"max-memory": "desc"}]  <==== multiple orders
         },
         "aggs": {
           "max-cpu": { "max": { "field": "cpu" } },
           "max-memory": { "max": { "field": "memory" } }
         }
       }
     }
}

Meanwhile, I think we also need to update the official document in: https://opensearch.org/docs/latest/aggregations/bucket/multi-terms/

@dblock
Copy link
Member

dblock commented Apr 25, 2024

I am not very familiar with this functionality, but I think we can work through this!

  1. Find an existing YAML REST test that has 1 order.
  2. See if there's a test with 2 orders, if not add one with expected change in results.
  3. Now that (1) and (2) are confirmed with tests we can be sure, and can document/update with cofidence.

@Zhikai-VM
Copy link
Contributor Author

I am not very familiar with this functionality, but I think we can work through this!

  1. Find an existing YAML REST test that has 1 order.
  2. See if there's a test with 2 orders, if not add one with expected change in results.
  3. Now that (1) and (2) are confirmed with tests we can be sure, and can document/update with cofidence.

Hi @dblock ,
I created a PR to add unit test to verify the multiple orders in multiple terms aggregation, could you please help to review it:
#13400 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants