Skip to content

Conversation

@jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Aug 21, 2019

The pick table compares the smallest and biggest key of each table
against the iteratorOptions.Prefix. The smallest and biggest key contain
timestamps while the prefix does not contain a timestamp. So pickTable
would return not return the correct tables.

For example
Table has Smallest [0 255 255... ] and prefix [0, 0, 1], the comparison
of these two values would return true implying that the smallest is
greater than the prefix which is incorrect because prefix doesn't
contains a timestamp and smallest does.

With this commit, we compare the smallest/biggest keys without
timestamps to the opt.Prefix.

Fixes #992


This change is Reviewable

The pick table compares the smallest and biggest key of each table
against the iteratorOptions.Prefix. The smallest and biggest key contain
timestamps while the prefix does not contains timestamp. So pickTable
would return not return the correct tables.

For example
Table has Smallest [0 255 255... ] and prefix [0, 0, 1], the comparison
of these two values would return true implying that the smallest is
greater than the prefix which is incorrect because prefix doesn't
contains a timestamp and smallest does.

With this commit, we compare the smallest/biggest keys without
timestamps to the opt.Prefix.

See #992 for more details.
@jarifibrahim jarifibrahim requested a review from a team August 21, 2019 12:46
Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

✅ A review job has been created and sent to the PullRequest network.


@jarifibrahim you can click here to see the review status or cancel the code review job.

@ashish-goswami
Copy link
Contributor

Just fixed this in #983

@ashish-goswami
Copy link
Contributor

Just check why travis is failing for Go1.11.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm: Did you add a test covering the bug, @jarifibrahim ?

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @manishrjain)

Copy link

@pullrequest pullrequest bot left a comment

Choose a reason for hiding this comment

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

Thank you for updating the comment and finding this bug. I have no additional feedback for this PR.


Reviewed with ❤️ by PullRequest

@jarifibrahim
Copy link
Contributor Author

@manishrjain Added a regression test.

@jarifibrahim
Copy link
Contributor Author

Just check why travis is failing for Go1.11.

The test for checksum mismatch was failing but I reran and it passed. Looks like we have a flaky test.

@jarifibrahim jarifibrahim merged commit 5f64ecf into master Aug 22, 2019
@jarifibrahim jarifibrahim deleted the ibrahim/pickTable branch August 22, 2019 09:49
jarifibrahim pushed a commit that referenced this pull request Mar 12, 2020
The pick table compares the smallest and biggest key of each table
against the iteratorOptions.Prefix. The smallest and biggest key contain
timestamps while the prefix does not contains timestamp. So pickTable
would not return the correct tables.

For example
Table has Smallest [0 255 255... ] and prefix [0, 0, 1], the comparison
of these two values would return true implying that the smallest is
greater than the prefix which is incorrect because prefix doesn't
contains a timestamp and smallest does.

With this commit, we compare the smallest/biggest keys without
timestamps to the opt.Prefix.

See #992 for more details.

(cherry picked from commit 5f64ecf)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Using badger.IteratorOptions.Prefix can hide data if certain keys have been written

4 participants