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

Add geodistance query test #427

Merged
merged 5 commits into from
Jul 18, 2024

Conversation

kolchfa-aws
Copy link
Contributor

Description

Adds a spec test for the geodistance query

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.

Copy link
Contributor

github-actions bot commented Jul 16, 2024

Changes Analysis

Commit SHA: 1781882
Comparing To SHA: b553c4b

API Changes

Summary


└─┬Components
  └─┬_common.query_dsl:GeoDistanceQuery
    └─┬ALLOF
      ├──[➕] required (35079:15)❌ 
      ├──[➕] properties (35075:13)
      └──[➕] properties (35069:13)

Document Element Total Changes Breaking Changes
components 3 2
  • BREAKING Changes: 2 out of 3
  • Additions: 3
  • Breaking Additions: 1

Report

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

API Coverage

Before After Δ
Covered (%) 483 (47.31 %) 483 (47.31 %) 0 (0 %)
Uncovered (%) 538 (52.69 %) 538 (52.69 %) 0 (0 %)
Unknown 24 24 0

@kolchfa-aws
Copy link
Contributor Author

kolchfa-aws commented Jul 16, 2024

@nhtruong @Xtansia @dblock This is my first spec test so please bear with me :) The geodistance query requires a field from which to measure the distance. This field must be a geo_point. If I add a field to the test without adding it to the spec, it fails because the field is not in the spec. If I add the field property to the GeoDistanceQuery, I cannot specify the field to be geo_point (it's not one of the allowed values). What's the preferred way of going about this?

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.

Looks great! A few asks.

This is really search, and we'll have more, so let's make a new folder tests/_core/search, and call this file geo_distance.yaml to match the query type? If you want to break up search.yaml along those lines in the same PR go for it too, otherwise I can do it later.

@@ -625,8 +625,16 @@ components:
$ref: '_common.yaml#/components/schemas/GeoDistanceType'
validation_method:
$ref: '#/components/schemas/GeoValidationMethod'
ignore_unmapped:
Copy link
Member

Choose a reason for hiding this comment

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

Add to CHANGELOG. This is a new field added.

Do you know which version this new option was added in? Might need x-added-....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this field has been inherited since 1.0. Is there an easy way to make sure (other than running the query against 1.0 :)?

Copy link
Member

Choose a reason for hiding this comment

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

I usually look at code. We now have 1.3.x tests here, so this will fail if it didn't exist.

spec/schemas/_common.query_dsl.yaml Show resolved Hide resolved
tests/_core/geodistance_query.yaml Outdated Show resolved Hide resolved
@kolchfa-aws
Copy link
Contributor Author

@dblock Thanks for reviewing. I addressed your comments.

@dblock
Copy link
Member

dblock commented Jul 17, 2024

@kolchfa-aws Can you please rebase?

dblock
dblock previously approved these changes Jul 17, 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.

Looks good! Just needs a rebase.

@@ -625,8 +625,16 @@ components:
$ref: '_common.yaml#/components/schemas/GeoDistanceType'
validation_method:
$ref: '#/components/schemas/GeoValidationMethod'
ignore_unmapped:
Copy link
Member

Choose a reason for hiding this comment

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

I usually look at code. We now have 1.3.x tests here, so this will fail if it didn't exist.

director: Bennett Miller
title: Moneyball
year: 2011

Copy link
Member

Choose a reason for hiding this comment

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

Get rid of empty trailing line.

@dblock
Copy link
Member

dblock commented Jul 17, 2024

@nhtruong @Xtansia @dblock This is my first spec test so please bear with me :) The geodistance query requires a field from which to measure the distance. This field must be a geo_point. If I add a field to the test without adding it to the spec, it fails because the field is not in the spec. If I add the field property to the GeoDistanceQuery, I cannot specify the field to be geo_point (it's not one of the allowed values). What's the preferred way of going about this?

Looks like you figured out a workaround by putting this in the prologue? LMK if you still need help.

@dblock
Copy link
Member

dblock commented Jul 17, 2024

Looks like there are still unrelated changes and a missing DCO. LMK if you need help.

Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
Signed-off-by: Fanit Kolchina <kolchfa@amazon.com>
Signed-off-by: Daniel (dB.) Doubrovkine <dblock@amazon.com>
@dblock dblock merged commit 51cb05a into opensearch-project:main Jul 18, 2024
11 checks passed
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.

2 participants