Changed SearchAfter field type to FieldValue instead of String #769
Conversation
channel-dante
requested review from
reta,
Bukhtawar,
dblock,
szczepanczykd,
madhusudhankonda,
saratvemulapalli and
VachaShah
as code owners
channel-dante
force-pushed
the
after-search-type
branch
from
248dde4 to
15e6770
Compare
channel-dante
changed the title
channel-dante
force-pushed
the
after-search-type
branch
from
e3b9696 to
a24b011
Compare
There was a problem hiding this comment.
So, existing code that was .searchAfter("string") worked, right? Therefore after this change that code would no longer build?
This would make the change backwards incompatible, which would require a major version bump. Can we implement it in a way that doesn't break backwards compat?
CHANGELOG.md
Outdated
| @@ -42,6 +42,7 @@ This section is for maintaining a changelog for all breaking changes for the cli | |||
| ### Dependencies | |||
|
|
|||
| ### Changed | |||
| - Changed SearchAfter of SearchRequest type to FieldValue instead of String | |||
There was a problem hiding this comment.
Add a link to the PR like other line-items.
There was a problem hiding this comment.
Since overload cannot distinguish between List<FieldValue> and List<String>, I think you have no choice but to make backwards incompatible. It can be implemented as shown below, but it seems that there will be a side effect of allowing unintended Object type.
And by adding this overloading, the issue of not being able to find the targeted method in the deserializer also occurs.
There was a problem hiding this comment.
@reta any ideas what's best here?
There was a problem hiding this comment.
@reta Can you give me any comment here? 🥲
There was a problem hiding this comment.
Sorry folks, overlooked this pull request, we could add the varargs overload:
public final Builder searchAfter(String value, String... values) {
but indeed due to erasure, we may only keep List<String> for backward compatibility in 2.x (and convert it to FielValue internally) but use List<FieldValue> in 3.x, what do you think?
There was a problem hiding this comment.
@reta Converting to FieldValue only internally doesn't seem to make sense. So, how about applying this PR to 3.x? If I have to do that, should I move the branch to 3.x?
There was a problem hiding this comment.
Sure @channel-dante , the main is 3.x so no need to change anything
|
I think this PR looks good, it's a breaking change for 3.0. For a backport to 2.x your plan LGTM. How about we start introducing an UPGRADING.md that describes all things that change in a way that is backwards incompatible? @channel-dante Care to add an UPGRADING.md in the same format as https://github.com/opensearch-project/opensearch-py/blob/main/UPGRADING.md? And rebase. Thank you for hanging in here with us! |
channel-dante
force-pushed
the
after-search-type
branch
from
ac86604 to
889f789
Compare
UPGRADING.md
Outdated
| ## [UPGRADING 2.x to 3.0] | ||
| ### SearchAfter of SearchRequest type | ||
| - Changed SearchAfter of SearchRequest type to FieldValue instead of String | ||
| - Consider using `FieldValue.of("")` to make string type values compatible. |
There was a problem hiding this comment.
| - Consider using `FieldValue.of("")` to make string type values compatible. | |
| - Consider using `FieldValue.of` to make string type values compatible. |
…ring Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
Signed-off-by: channel-dante <[email protected]>
channel-dante
force-pushed
the
after-search-type
branch
from
2dde48a to
d6a4532
Compare
|
@dblock Can you please confirm about this pr? |
|
Merged, thanks @channel-dante. Want to do a manual backport for a backwards compatible variation? |
|
@dblock I didn't understand what kind of work it was. Can you give a little explanation? |
|
Nevermind, I re-read the comments on bcw above. Thanks. |
Description
Issues Resolved
#755
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.