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 integer range condition and integer start_from to interface #3574

Closed
wants to merge 9 commits into from

Conversation

coszio
Copy link
Contributor

@coszio coszio commented Feb 9, 2024

To finish taking advantage of the newly added range interface. This adds:

  • Range<IntPayloadType> to RangeInterface
  • StartFrom::Int(IntPayloadType) variant for order-by scroll requests.

All Submissions:

  • Contributions should target the dev branch. Did you create your branch from dev?
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you formatted your code locally using cargo +nightly fmt --all command prior to submission?
  3. Have you checked your code using cargo clippy --all --all-features command?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@coszio
Copy link
Contributor Author

coszio commented Feb 12, 2024

Update: merge conflicts have been solved

@@ -59,6 +59,34 @@ impl DateTimeWrapper {
}
}

impl FromStr for DateTimePayloadType {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part was just moved closer to the definition of DateTimeWrapper

Comment on lines 150 to 167
impl ValueChecker for Range<FloatPayloadType> {
fn check_match(&self, payload: &Value) -> bool {
payload
.as_number()
.and_then(|x| x.as_f64().or_else(|| x.as_i64().map(|x| x as f64)))
.map_or(false, |x| self.check_range(x))
}
}

impl ValueChecker for Range<IntPayloadType> {
fn check_match(&self, payload: &Value) -> bool {
payload
.as_number()
.and_then(|x| x.as_i64().or_else(|| x.as_f64().map(|x| x as i64)))
.map_or(false, |x| self.check_range(x))
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For unindexed fields, we now try to convert into the type of the range to do the checks

Comment on lines 249 to 270
pub fn get_int_range_checkers(
index: &FieldIndex,
range: Range<IntPayloadType>,
) -> Option<ConditionCheckerFn> {
match index {
FieldIndex::IntIndex(num_index) => Some(Box::new(move |point_id: PointOffsetType| {
num_index
.get_values(point_id)
.is_some_and(|values| values.iter().copied().any(|i| range.check_range(i)))
})),
FieldIndex::FloatIndex(num_index) => {
let range = range.map(|i| i as FloatPayloadType);
Some(Box::new(move |point_id: PointOffsetType| {
num_index
.get_values(point_id)
.is_some_and(|values| values.iter().copied().any(|f| range.check_range(f)))
}))
}
_ => None,
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For indexed fields, we follow same approach as before, converting the range into the type of the index

@coszio
Copy link
Contributor Author

coszio commented Feb 16, 2024

I am facing issues making appropriate conversions in the python client (in which pydantic is matching floats to ints, and viceversa). Maybe this is a good reason to not include these changes.

In the meantime, I'll draft this PR to avoid merging just yet.

Edit: this is no longer an issue with pydantic >= 2, which uses smart union matching. Undrafting

@coszio coszio marked this pull request as draft February 16, 2024 21:01
@coszio coszio marked this pull request as ready for review February 19, 2024 14:24
xzfc
xzfc previously approved these changes Feb 20, 2024
@coszio coszio dismissed xzfc’s stale review March 21, 2024 17:37

Dismissing the review to avoid merging yet. The current approach is problematic with client generation. Specifically when deciding if a range is represented as all int or all float, a better approach might be to allow this flexibility at the comparator level. E.g gt: IntOrFloat

@generall generall marked this pull request as draft May 27, 2024 11:21
@generall
Copy link
Member

Please feel free to make another iteration, once we will have a better idea of how integrate it into the client

@generall generall closed this May 27, 2024
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.

None yet

3 participants