Added aggregation tests for avg, max, min, range, terms, sum, nested and histogram.

[kkewwei]

Description

Describe what this change achieves.

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

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.

[github-actions]

Changes Analysis

Commit SHA: 1ce95ea
Comparing To SHA: 7d05664

API Changes

Summary

└─┬Components
  └─┬_common.aggregations:NestedAggregate
    └─┬ALLOF
      └──[➕] properties (34326:13)

Document Element	Total Changes	Breaking Changes
components	1	0

• Total Changes: 1
• Additions: 1

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/11094040873/artifacts/1992639674

API Coverage

	Before	After	Δ
Covered (%)	588 (57.59 %)	588 (57.59 %)	0 (0 %)
Uncovered (%)	433 (42.41 %)	433 (42.41 %)	0 (0 %)
Unknown	25	25	0

[dblock]

Thanks!

Looks like tests failed on 2.0, possibly because the feature wasn't there, so the test needs to be annotated accordingly.

Run npm run lint--fix for the linter to be happy.

I have some more nitpicks below.

[dblock]

Some tests against 2.x failed, probably needs a version: somewhere.

[dblock]

ERROR   _core/search/aggs/nested_agg.yaml (Invalid Story: data/prologues/0 contains unsupported properties: request_body, request_body, request_body --- data/chapters/0/synopsis must match pattern "^\p{Lu}[\s\S]*\.$")

It's request/payload.

[kkewwei]

@dblock Thank you for your patient review, I'm not familiar with opensearch-api-specification, Is there any command I can use to test the change? npm run test is ok?

[dblock]

@dblock Thank you for your patient review, I'm not familiar with opensearch-api-specification, Is there any command I can use to test the change? npm run test is ok?

The npm run test just runs unit and integration tests for tools, but not the actual spec tests (maybe it should).

Take a moment to read https://github.com/opensearch-project/opensearch-api-specification/blob/main/DEVELOPER_GUIDE.md and https://github.com/opensearch-project/opensearch-api-specification/blob/main/TESTING_GUIDE.md. I know it's a lot.

We test an entire matrix of versions of OpenSearch, the 2.0 tests are failing. If you want to run those locally, it's basically starting a container with OPENSEARCH_VERSION=2.0.0 docker compose up from tests/default and then running tests with OPENSEARCH_PASSWORD=admin npm run test:spec.

[kkewwei]

@dblock sorry for the latency reply(a bit busy), I will continue to follow up now.

[dblock]

Thanks! LMK when it's ready to be reviewed.

Some of this should work in 2.17, right?

[kkewwei]

Thanks! LMK when it's ready to be reviewed.

Some of this should work in 2.17, right?

@dblock thanks for your help, I know how to test.

nested aggregation can work in 2.x, so I delete the version.

When I test the aggregation, it throws the exception:

            FAILED  RESPONSE PAYLOAD SCHEMA (data/aggregations/title MUST contain the missing properties: value, values, values, values, values, values, value, value, value, value, value, value, value, value, value, value, keys, avg, count, max, min, sum, avg, count, max, min, sum, avg, count, max, m, ...)

It seems that I should define the response schema of the aggregation, I try a long time to define user-input aggs1 but fails, can you help adding a example aggregation test, and let me learn?

[dblock]

Given the search response:

    search@200:
      content:
        application/json:
          schema:
            $ref: '../schemas/_core.search.yaml#/components/schemas/ResponseBody'

it's a ResponseBody which defines aggregations:.

        aggregations:
          type: object
          additionalProperties:
            $ref: '_common.aggregations.yaml#/components/schemas/Aggregate'

if we replace this with just a vanilla object with any property (additionalProperties: true) the test will pass, so we're in the right place.

        aggregations:
          type: object
          additionalProperties: true
          # additionalProperties:
          #   $ref: '_common.aggregations.yaml#/components/schemas/Aggregate'

An Aggregate is one of many things:

    Aggregate:
      oneOf:
        - $ref: '#/components/schemas/CardinalityAggregate'
        - $ref: '#/components/schemas/HdrPercentilesAggregate'
        - ...

If we make the aggregate match our return value the spec also passes.

    Aggregate:
      oneOf:
        - type: object
          properties:
            doc_count:
              type: number

But if we add the other possibilities, it fails!

      oneOf:
        - type: object
          properties:
            doc_count:
              type: number
        - $ref: '#/components/schemas/CardinalityAggregate'
        - $ref: '#/components/schemas/HdrPercentilesAggregate'

This is where I've had a 🤦 moment multiple times. Turns out that oneOf behaves like "only one of", not "any one of", so if more than one aggregate type matches, it will fail. And the schema validator is not smart enough to say "your object matches N items, but expected to only match 1".

The fix is to change oneOf by anyOf.

    Aggregate:
      anyOf:
        - $ref: '#/components/schemas/CardinalityAggregate'
        - $ref: '#/components/schemas/HdrPercentilesAggregate'

Note that each aggregation correctly uses allOf, combining aggregateBase and other properties of a specific aggregation. For example:

    PercentilesAggregateBase:
      allOf:
        - $ref: '#/components/schemas/AggregateBase'
        - type: object
          properties:
            values:
              $ref: '#/components/schemas/Percentiles'
          required:
            - values

dblock@e497dea

Rebase, then merge https://github.com/dblock/opensearch-api-specification/tree/kkewwei-add_nested_aggs onto your branch? I was on a plane and added some more agg tests on top.

[github-actions]

Spec Test Coverage Analysis

Total	Tested
500	303 (60.6 %)

[dblock]

Let's add a 1-liner to CHANGELOG, maybe "Fix /{index}/_search with aggregations"?

Link checker is failing not sure how to fix that ... unrelated but if you have an idea include that.

[kkewwei]

"Link checker is failing" seems to be a probabilistic problem, It is caused by "Network error: Too Many Requests", I have no idea how to fix it.