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 deltaiterator upperbound check #11662

Closed
wants to merge 6 commits into from

Conversation

cz2h
Copy link
Contributor

@cz2h cz2h commented Aug 1, 2023

Fixes issue 11606.
Fixes #2343.

Fix problem of WBWIIteratorImpl not disabling itself when it's delta iterator is at entry out of upperbound.

@jsteemann
Copy link
Contributor

Seems to fix the issue described in #2343 (iterate_upper_bound not honored).

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@facebook-github-bot
Copy link
Contributor

@cz2h has updated the pull request. You must reimport the pull request before landing.

@cz2h
Copy link
Contributor Author

cz2h commented Aug 2, 2023

@ajkr @jsteemann Many thanks for the quick review. Kindly format the code to pass the last styling test.

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@@ -307,7 +312,8 @@ void BaseDeltaIterator::UpdateCurrent() {
if (comparator_->CompareWithoutTimestamp(
delta_entry.key, /*a_has_ts=*/false, *iterate_upper_bound_,
/*b_has_ts=*/false) >= 0) {
// out of upper bound -> finished.
// out of upper bound -> delta finished.
delta_iterator_out_of_bound_ = true;
Copy link
Member

Choose a reason for hiding this comment

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

Should we reset delta_iterator_out_of_bound_ to false when a user calls Seek* again?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point... we should probably pass the upper_bound down to the WBWIIteratorImpl and have it do the upper-bound check normally, i.e., by returning Valid() == false upon reaching the upper-bound.

@cbi42
Copy link
Member

cbi42 commented Aug 3, 2023

Another usage of iterate_upper_bound in db iterator is that SeekToLast() will seek to the first key before it. To make WBWI iterator consistent with base iterator behavior, we probably need to also implement that as a follow up.

@facebook-github-bot
Copy link
Contributor

@cz2h has updated the pull request. You must reimport the pull request before landing.

@cz2h
Copy link
Contributor Author

cz2h commented Aug 7, 2023

Hi @ajkr , I think the this pr provides a more detailed solution in fixing these series of issues related to the bounds of WBIIIterator and I am ok to close this one.

In the meantime could you provide some comments/thoughts on this fix, thanks.

@ajkr ajkr closed this Aug 7, 2023
@cz2h cz2h deleted the add-deltaiterator-upperbound-check branch August 27, 2023 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants