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

handle user input on get_docid_for_value_range #1760

Merged
merged 2 commits into from
Jan 12, 2023
Merged

Conversation

PSeitz
Copy link
Contributor

@PSeitz PSeitz commented Jan 5, 2023

fixes #1757

@PSeitz PSeitz force-pushed the get_docs_for_val_bug branch from 1406d82 to 5dd47cd Compare January 5, 2023 06:24
@codecov-commenter
Copy link

Codecov Report

Merging #1760 (5dd47cd) into main (b22f966) will increase coverage by 0.00%.
The diff coverage is 95.05%.

@@           Coverage Diff            @@
##             main    #1760    +/-   ##
========================================
  Coverage   94.13%   94.14%            
========================================
  Files         267      267            
  Lines       50900    51129   +229     
========================================
+ Hits        47917    48136   +219     
- Misses       2983     2993    +10     
Impacted Files Coverage Δ
fastfield_codecs/src/monotonic_mapping_u128.rs 100.00% <ø> (ø)
fastfield_codecs/src/serialize.rs 86.56% <75.00%> (ø)
fastfield_codecs/src/monotonic_mapping.rs 92.00% <75.86%> (-8.00%) ⬇️
fastfield_codecs/src/column.rs 93.62% <95.45%> (-0.06%) ⬇️
fastfield_codecs/src/compact_space/mod.rs 96.76% <100.00%> (-0.17%) ⬇️
fastfield_codecs/src/lib.rs 98.91% <100.00%> (+0.01%) ⬆️
src/fastfield/mod.rs 99.76% <100.00%> (+0.03%) ⬆️
src/fastfield/serializer/mod.rs 100.00% <100.00%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@PSeitz PSeitz requested a review from fulmicoton January 5, 2023 07:42
///
/// coerce_up means the next valid upper value in the value space will be chosen if the value
/// has to be coerced.
fn mapping_coerce(&self, inp: External, _coerce_up: bool) -> (bool, Internal) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would something like .map_lower_than(&self, bound: Bound<To>) -> Bound<From> do the job?

If it would, I feel like the contract is more obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could pass a Range in mapping_coerce instead (we don't have bounds here anymore). We still need the early exit for correctness if both map in front of the value space, and for that return a 1..0 range.

Copy link
Collaborator

@fulmicoton fulmicoton left a comment

Choose a reason for hiding this comment

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

See comment inline

@PSeitz PSeitz force-pushed the get_docs_for_val_bug branch 3 times, most recently from 0ddaa81 to e30f74b Compare January 9, 2023 11:46
@PSeitz PSeitz force-pushed the get_docs_for_val_bug branch from e30f74b to 5ea89f5 Compare January 10, 2023 11:54
@PSeitz PSeitz force-pushed the get_docs_for_val_bug branch from 5ea89f5 to 8962cfd Compare January 12, 2023 09:34
@PSeitz PSeitz merged commit 1176555 into main Jan 12, 2023
@PSeitz PSeitz deleted the get_docs_for_val_bug branch January 12, 2023 13:20
fulmicoton pushed a commit that referenced this pull request Jan 16, 2023
* handle user input on get_docid_for_value_range

fixes #1757

* pass range as parameter
Hodkinson pushed a commit to Hodkinson/tantivy that referenced this pull request Jan 30, 2023
* handle user input on get_docid_for_value_range

fixes quickwit-oss#1757

* pass range as parameter
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.

get_docids_for_value_range is broken w/certain fast fields that use a GCD inverse
3 participants