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 suggestion classes for Term, Phrase, and Completion. #477

Merged
merged 7 commits into from
Jun 14, 2023

Conversation

harshavamsi
Copy link
Contributor

@harshavamsi harshavamsi commented May 11, 2023

Description

This change refactors the suggestion classes for Term, Phrase, and Completion suggestion.

The previous logic used theUnionDeserializer class to deserialize the responses from these API calls. But due to a bug in the way the UnionDeserializer treats member object, phrase completion was broken. This PR uses an alternative approach with ExternallyTaggedUnionDeserializer. It is correctly able to differentiate between the three entities.

Further, also refactored the Suggester classes for the three entities. The API is now more verbose and has the correct calling order. The previous was of accepting the options provided by the response was response.suggest().get(suggesterName).get(0).options().get(0).phrase().text. It is now response.suggest().get(suggesterName).get(0).phrase().options().get(0).text, which is more logically.

Issues Resolved

Closes #459

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.

@harshavamsi harshavamsi force-pushed the field-suggester-fix branch 2 times, most recently from eb11832 to 04f306a Compare May 11, 2023 01:03
@harshavamsi harshavamsi changed the title Field suggester fix Fix suggestion classes for Term, Phrase, and Completion. May 11, 2023
Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>
USER_GUIDE.md Outdated Show resolved Hide resolved
@dblock
Copy link
Member

dblock commented May 11, 2023

Looks like a breaking change and will need to be in a major version increment?

@harshavamsi
Copy link
Contributor Author

Looks like a breaking change and will need to be in a major version increment?

I don't think this would classify as a breaking change, this is a bug fix for broken feature, see #459. The refactoring of the core logic around the way functions are used is only because it was incorrect in the first place. But looks like there are no folks using it already because it was completely broken since fork anyway.

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>
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. Found some nits.

I think you should write unit tests for the new classes, even if it's very basic stuff.

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>
@harshavamsi
Copy link
Contributor Author

harshavamsi commented May 15, 2023

For the purposes of escalating customer requests, I think that we should get this change in and release in 2.5.0. I think we have some confidence given that the integration tests are passing. I'm going to open up a new issue to add unit tests to the classes we've added(I don't believe there are unit tests for any API classes today). Opened #481

@harshavamsi
Copy link
Contributor Author

@reta @VachaShah @szczepanczykd can you also review?

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>
USER_GUIDE.md Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>
USER_GUIDE.md Outdated Show resolved Hide resolved
@harshavamsi
Copy link
Contributor Author

@reta I can fix the suggestions on another PR, for the time being can we merge this?

@harshavamsi
Copy link
Contributor Author

@dblock @VachaShah need approvals

@reta
Copy link
Collaborator

reta commented Jun 13, 2023

@reta I can fix the suggestions on another PR, for the time being can we merge this?

Why another PR?

@harshavamsi
Copy link
Contributor Author

#459 -- seems like some folks really want this fix

@reta
Copy link
Collaborator

reta commented Jun 13, 2023

#459 -- seems like some folks really want this fix

I can help you address the comments, the documentation is the way we recommend our users to use the Java client, it should showcase the right APIs

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>
@harshavamsi
Copy link
Contributor Author

@reta updated the example and tests to use builders instead

@dblock
Copy link
Member

dblock commented Jun 14, 2023

#459 -- seems like some folks really want this fix

I'm glad we're doing something users want! Let's make sure to address the must have's :) @reta all yours for CR!

java-client/build.gradle.kts Outdated Show resolved Hide resolved
Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>
@reta reta merged commit 68e2b6f into opensearch-project:main Jun 14, 2023
@reta reta added the backport 2.x Backport to 2.x branch label Jun 14, 2023
@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-477-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 68e2b6f03ff47ced215f600a7c9f5dd28b0d4595
# Push it to GitHub
git push --set-upstream origin backport/backport-477-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-477-to-2.x.

@reta
Copy link
Collaborator

reta commented Jun 14, 2023

@harshavamsi could you please manually backport to 2.x? thank you

harshavamsi added a commit to harshavamsi/opensearch-java that referenced this pull request Jun 14, 2023
…project#477)

* Fixes Completion, Phrase, and Term suggesters

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Use resource files instead of JSON strings

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Minor fixes to docs + checksytle

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Adding ES license headers for copied over files

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Fix example formatting

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Update tests to use builders instead of JSON

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Removing un-necessary test resources

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

---------

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>
(cherry picked from commit 68e2b6f)
harshavamsi added a commit to harshavamsi/opensearch-java that referenced this pull request Jun 14, 2023
…project#477)

* Fixes Completion, Phrase, and Term suggesters

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Use resource files instead of JSON strings

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Minor fixes to docs + checksytle

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Adding ES license headers for copied over files

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Fix example formatting

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Update tests to use builders instead of JSON

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Removing un-necessary test resources

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

---------

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>
(cherry picked from commit 68e2b6f)
VachaShah pushed a commit that referenced this pull request Jun 14, 2023
* Fixes Completion, Phrase, and Term suggesters

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Use resource files instead of JSON strings

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Minor fixes to docs + checksytle

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Adding ES license headers for copied over files

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Fix example formatting

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Update tests to use builders instead of JSON

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

* Removing un-necessary test resources

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>

---------

Signed-off-by: Harsha Vamsi Kalluri <harshavamsi096@gmail.com>
(cherry picked from commit 68e2b6f)
@iamdanhart
Copy link
Contributor

Just as an anecdotal data point, this breaks one of my usages of the client library around completion suggestion (works as intended currently on version 2.4.0). It should be a relatively light lift for me to fix, but not something I expected in a minor release bump.

@dblock
Copy link
Member

dblock commented Jul 28, 2023

Just as an anecdotal data point, this breaks one of my usages of the client library around completion suggestion (works as intended currently on version 2.4.0). It should be a relatively light lift for me to fix, but not something I expected in a minor release bump.

Yep, shouldn't have per semver. What's your before/after example? You should open a bug for it.

@iamdanhart
Copy link
Contributor

Note between the two examples, a few class names appeared to change and order of things to get to the source document changed.

Snippet of beginning to process my response from 2.4:

SearchResponse<MyType> response = this.openSearchClient.search(suggestRequest, MyType.class);

response.suggest().forEach((key, value) -> {
    for (Suggestion<MyType> s : value) {
        for (SuggestOption<MyType> so : s.options()) {
            MyType myType = so.completion().source();
            ...

From 2.6:

SearchResponse<MyType> response = this.openSearchClient.search(suggestRequest, MyType.class);

response.suggest().forEach((key, value) -> {
    for (Suggest<MyType> s : value) {
        CompletionSuggest<MyType> suggest = s.completion();
        for (CompletionSuggestOption<MyType> so : suggest.options()) {
            MyType myType = so.source();
              ...

@dblock
Copy link
Member

dblock commented Aug 15, 2023

@harshavamsi WDYT?

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] Request failed: [parse_exception] missing suggestion object
5 participants