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

fix issues for termsQuery #570

Merged
merged 6 commits into from
Sep 16, 2024
Merged

Conversation

amberzsy
Copy link
Contributor

@amberzsy amberzsy commented Sep 12, 2024

Description

couple of issues here:

  1. missing top level param boost as defined here
  2. missing description for terms Query.
  3. missing terms look up test https://opensearch.org/docs/latest/query-dsl/term/terms/#terms-lookup
additionalProperties: with 
minProperties: 1
maxProperties: 1

should be simplified with oneOf.
5. incorrect definition for terms look up query, boost is not valid field so should not have QueryBase as part of TermsQuery here

Issues Resolved

fixes #540

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.

@Xtansia
Copy link
Collaborator

Xtansia commented Sep 12, 2024

@amberzsy Are you able to provide an example (i.e. dashboards dev tools POST) of the query you're trying to represent? Preferably also adding it to https://github.com/opensearch-project/opensearch-api-specification/blob/main/tests/default/_core/search/query/terms.yaml

Copy link
Contributor

github-actions bot commented Sep 12, 2024

Changes Analysis

Commit SHA: ab96677
Comparing To SHA: c2c666f

API Changes

Summary

└─┬Components
  ├──[➖] schemas (39605:7)❌ 
  ├──[➕] schemas (39609:7)
  ├──[➕] schemas (39598:7)
  └─┬_common.query_dsl:QueryContainer
    └─┬terms
      └──[🔀] $ref (39609:13)❌ 

Document Element Total Changes Breaking Changes
components 4 2
  • BREAKING Changes: 2 out of 4
  • Modifications: 1
  • Removals: 1
  • Additions: 2
  • Breaking Removals: 1
  • Breaking Modifications: 1

Report

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

API Coverage

Before After Δ
Covered (%) 560 (54.85 %) 560 (54.85 %) 0 (0 %)
Uncovered (%) 461 (45.15 %) 461 (45.15 %) 0 (0 %)
Unknown 25 25 0

Copy link
Contributor

github-actions bot commented Sep 12, 2024

Spec Test Coverage Analysis

Total Tested
585 290 (49.57 %)

@amberzsy
Copy link
Contributor Author

amberzsy commented Sep 12, 2024

@amberzsy Are you able to provide an example (i.e. dashboards dev tools POST) of the query you're trying to represent? Preferably also adding it to https://github.com/opensearch-project/opensearch-api-specification/blob/main/tests/default/_core/search/query/terms.yaml

sure added.
image

@nhtruong
Copy link
Collaborator

Tests failed

nhtruong
nhtruong previously approved these changes Sep 12, 2024
Copy link
Collaborator

@nhtruong nhtruong left a comment

Choose a reason for hiding this comment

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

please fix the tests

Signed-off-by: amberzsy <xxamber998@gmail.com>
Signed-off-by: amberzsy <xxamber998@gmail.com>
…as boost/_name not valid 3. add TermsLookup test

Signed-off-by: amberzsy <xxamber998@gmail.com>
Signed-off-by: amberzsy <xxamber998@gmail.com>
@amberzsy
Copy link
Contributor Author

amberzsy commented Sep 15, 2024

please fix the tests

done.
Also, noticed the public documentation seems incorrect here. [OpenSearch version - 2.16]
image

fix ^^ issue in latest commit.

@amberzsy amberzsy changed the title fix missing top level boost param for termsQuery fix issues for termsQuery Sep 15, 2024
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

This looks good. The linter failed, run npm run lint--fix.

tests/default/_core/search/query/terms.yaml Outdated Show resolved Hide resolved
tests/default/_core/search/query/terms.yaml Outdated Show resolved Hide resolved
Signed-off-by: amberzsy <xxamber998@gmail.com>
Signed-off-by: amberzsy <xxamber998@gmail.com>
@amberzsy amberzsy requested a review from dblock September 16, 2024 07:02
@nhtruong nhtruong merged commit 811a5c9 into opensearch-project:main Sep 16, 2024
18 checks passed
dblock pushed a commit to dblock/opensearch-api-specification that referenced this pull request Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] missing definition in termsQuery
4 participants