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

feat(add-xy_shape): Ability to use xy_shape field type #885

Merged
merged 9 commits into from
Mar 19, 2024

Conversation

kmessaoudi
Copy link
Contributor

@kmessaoudi kmessaoudi commented Mar 7, 2024

Description

Currently when using the shape property with the opensearch java client, this result as an error as it is known to OpenSearch, where this feature has the name xy_shape.
We were unsure of shape usage and didn't know if we should have removed it.
This PR adds the ability of using Xy shape property.

Issues Resolved

Fixes #884

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>
Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>
CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>
Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>
Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>
reta
reta previously approved these changes Mar 18, 2024
Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>
Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>
reta
reta previously approved these changes Mar 18, 2024
@reta
Copy link
Collaborator

reta commented Mar 18, 2024

@dblock any concerns from your side? thank you!

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.

I see your conversation around license. The XyShapeQuery code looks suspiciously like the existing ShapeQuery code, so I am afraid we need to put back the Elastic license header in there. Even if they refer to two different concepts, I think we should be safe vs. sorry.

@kmessaoudi
Copy link
Contributor Author

Hi @dblock
Thank you for your answer
It is the same code, yes.
Is it that header you want me to put back ?

/*
 * Licensed to Elasticsearch B.V. under one or more contributor
 * license agreements. See the NOTICE file distributed with
 * this work for additional information regarding copyright
 * ownership. Elasticsearch B.V. licenses this file to you under
 * the Apache License, Version 2.0 (the "License"); you may
 * not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing,
 * software distributed under the License is distributed on an
 * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 * KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations
 * under the License.
 */

/*
 * Modifications Copyright OpenSearch Contributors. See
 * GitHub history for details.
 */

@reta
Copy link
Collaborator

reta commented Mar 19, 2024

Hi @dblock Thank you for your answer It is the same code, yes. Is it that header you want me to put back ?

/*
 * Licensed to Elasticsearch B.V. under one or more contributor
 * license agreements. See the NOTICE file distributed with
 * this work for additional information regarding copyright
 * ownership. Elasticsearch B.V. licenses this file to you under
 * the Apache License, Version 2.0 (the "License"); you may
 * not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing,
 * software distributed under the License is distributed on an
 * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
 * KIND, either express or implied.  See the License for the
 * specific language governing permissions and limitations
 * under the License.
 */

/*
 * Modifications Copyright OpenSearch Contributors. See
 * GitHub history for details.
 */

@kmessaoudi I think what @dblock means is that: for new files - the header is correct (SPDX one), but for modified files (which also included renamed like Shape -> XyShape) would be more correct to preserve the original license header.

@dblock
Copy link
Member

dblock commented Mar 19, 2024

Yes, please. Sorry for being annoying and appreciate your work!

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>
@@ -0,0 +1,14 @@
package org.opensearch.client.opensearch._types.query_dsl;
Copy link
Member

Choose a reason for hiding this comment

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

This one needs a license header too (only SPDX for the new file, but this one does look like a copy of the old ShapeFieldQueryTest. CI should be catching the missing one - if it's not and it's not a trivial fix, open a GH issue please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I've checked the other files in test directory before pushing, they don't have license header ?
It is indeed a copy of ShapeFieldQueryTest, the header wasn't there before, is that normal ?
I am not sure to understand

Copy link
Member

Choose a reason for hiding this comment

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

Sorry if I wasn't clear.

  1. It should have been there and CI should have caught this as a problem before.
  2. Please do add it, including the ES one since it's an old file.
  3. We need to make sure license checker catches this in CI, if the problem is not obvious (excluded folder or something), please open an issue?

Copy link
Collaborator

@reta reta Mar 19, 2024

Choose a reason for hiding this comment

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

It is indeed a copy of ShapeFieldQueryTest, the header wasn't there before, is that normal ?
I am not sure to understand

@kmessaoudi oh this seems like a miss on our side, thank you for pointing it out, I believe we need to have license headers there, @VachaShah may be it rings a bell to you?

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>
@dblock dblock merged commit ccdb56a into opensearch-project:main Mar 19, 2024
49 checks passed
@dblock dblock added the backport 2.x Backport to 2.x branch label Mar 19, 2024
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-885-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 ccdb56a31dca1184b29b3948ee66835df5681dd0
# Push it to GitHub
git push --set-upstream origin backport/backport-885-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-885-to-2.x.

@dblock
Copy link
Member

dblock commented Mar 19, 2024

@kmessaoudi I merged the PR. Looks like automatic backport failed, want to do a manual one to the 2.x branch so we can have this in a 2.x release?

kmessaoudi added a commit to kmessaoudi/opensearch-java that referenced this pull request Mar 19, 2024
…oject#885)

* feat(add-xy_shape): Ability to use xy_shape field type

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* update changelog

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* Fix changelog and add license header

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* updated license header

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* remove shape property

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* fix test after removing shape

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* adapt ShapeQuery test to XyShapeQuery

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* Add ES license headers

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* Add ES license headers on tests

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

---------

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>
(cherry picked from commit ccdb56a)
reta pushed a commit that referenced this pull request Mar 19, 2024
* feat(add-xy_shape): Ability to use xy_shape field type

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* update changelog

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* Fix changelog and add license header

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* updated license header

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* remove shape property

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* fix test after removing shape

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* adapt ShapeQuery test to XyShapeQuery

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* Add ES license headers

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

* Add ES license headers on tests

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>

---------

Signed-off-by: MESSAOUDI Khadidja <kmessaoudi@thefork.com>
(cherry picked from commit ccdb56a)
@BrendonFaleiro BrendonFaleiro mentioned this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Shape property doesn't work, should be XyShape
3 participants