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

Allow the use of percent string in Bool.__and__ method #780

Merged

Conversation

Godefroy-Amaury
Copy link
Contributor

Description

Allow the use of 50, "50" or "50%" as valid value for qx._min_should_match/min_should_match in the Bool.__and__ method.

Previously, if for any reason the qx._min_should_match was a string, a TypeError error was raise by the if len(qx.should) <= min_should_match:.

Still raise a ValueError if the given value is an invalid integer representation.

Issues Resolved

Closes #779

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.

@dblock
Copy link
Member

dblock commented Jul 18, 2024

Thanks! Add a test and a CHANGELOG entry, please?

It would be amazing if you could please look at https://github.com/opensearch-project/opensearch-api-specification as well and add tests there for this and other use-cases so we don't lose this functionality as we move towards more of this code being generated.

Copy link

codecov bot commented Jul 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.69%. Comparing base (ba715b9) to head (b786504).
Report is 42 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #780      +/-   ##
==========================================
- Coverage   71.95%   70.69%   -1.26%     
==========================================
  Files          91      105      +14     
  Lines        8001     8621     +620     
==========================================
+ Hits         5757     6095     +338     
- Misses       2244     2526     +282     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Looking pretty good, thanks for the tests. See below for comments.

Please do fix DCO, git commit --amend -s and force push will do it.

CHANGELOG.md Outdated
@@ -9,6 +9,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
### Removed
### Fixed
- Fixed Search helper to ensure proper retention of the _collapse attribute in chained operations. ([#771](https://github.com/opensearch-project/opensearch-py/pull/771))
- Fixed the use of `minimum_should_match` with `Bool` to allow the use of string-based value (percent string, combination)
Copy link
Member

Choose a reason for hiding this comment

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

Add PR number here.

min_should_match = qx._min_should_match

# attempt to convert a string integer representation to int
with contextlib.suppress(ValueError):
Copy link
Member

Choose a reason for hiding this comment

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

I am worried that int() may do unexpected things like rounding a float for example.

Maybe we can just be explicit, check that the value ends with % and convert the rest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But having a float at this point is a bit of nonsense, no? Looking at the documentions, the min_should_match mean "i need at least X values" - or did I miss something? Maybe a round up/down to the nearest should be ok? (something like: int(round(float("3.14"), 0)) -> 3)

Indeed, I can only check for string that end up with a "%", but we'll lose the possibility to use int who has been cast as string. I let you have the final word. 👍

Copy link
Member

@dblock dblock Jul 25, 2024

Choose a reason for hiding this comment

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

I think you misunderstood me, sorry.

I mean that if we rely on an exception to see if the value can be converted, then we might introduce undesired behavior.

$ python -c "print(int("3.14"))"
3

You can see that if I pass a value not accepted by the API it's happily coerced into an int, losing the actual value! That doesn't seem good :) It should be an error in this case, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, get it. If an invalid value is passed to the query, we don't want to magically (attempt to) fix it. 👍
Code and test has been updated.

@Godefroy-Amaury Godefroy-Amaury force-pushed the allow-percent-string branch 3 times, most recently from 5f75ff9 to 79d4d8b Compare July 25, 2024 16:23
Signed-off-by: Godefroy Amaury de Malefète <godefroy-de-montmirail-dit-le-hardi@protonmail.com>
@dblock
Copy link
Member

dblock commented Jul 25, 2024

Thanks for fixing this!

@dblock dblock merged commit a68e8b7 into opensearch-project:main Jul 25, 2024
60 of 62 checks passed
dblock pushed a commit to dblock/opensearch-py that referenced this pull request Aug 15, 2024
…oject#780)

Signed-off-by: Godefroy Amaury de Malefète <godefroy-de-montmirail-dit-le-hardi@protonmail.com>
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] TypeError error when using a .filter(...) on a Search object.
2 participants