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 plain table to store index on file with bloom filter disabled #1525

Closed
wants to merge 1 commit into from

Conversation

siying
Copy link
Contributor

@siying siying commented Nov 16, 2016

Summary:
Currently plain table bloom filter is required if storing metadata on file. Remove the constraint.

Test Plan: Relax existing unit test to cover this scenario.

Copy link
Contributor

@IslamAbdelRahman IslamAbdelRahman left a comment

Choose a reason for hiding this comment

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

Thanks @siying for fixing that !

@@ -308,7 +308,7 @@ Status PlainTableReader::PopulateIndex(TableProperties* props,
index_in_file &= s.ok();
Copy link
Contributor

Choose a reason for hiding this comment

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

So now index_in_file will be false even if the index block is in file but the bloom block is not ?
Does that mean that we don't get any benefit from storing the index in file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Let me fix it.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@siying updated the pull request - view changes - changes since last import

@facebook-github-bot
Copy link
Contributor

@siying updated the pull request - view changes - changes since last import

@siying
Copy link
Contributor Author

siying commented Nov 17, 2016

Some changes to make it the code easier to understand.

@facebook-github-bot
Copy link
Contributor

@siying updated the pull request - view changes - changes since last import

@siying
Copy link
Contributor Author

siying commented Nov 17, 2016

Revert a previous change in test, which is not needed now.

Summary:
Currently plain table bloom filter is required if storing metadata on file. Remove the constraint.

Test Plan: Add a unit test.
@facebook-github-bot
Copy link
Contributor

@siying updated the pull request - view changes - changes since last import

@siying
Copy link
Contributor Author

siying commented Nov 17, 2016

asan, ubsan failure, and the Travis failure don't seem to be related.

Copy link
Contributor

@IslamAbdelRahman IslamAbdelRahman 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 @siying

IslamAbdelRahman pushed a commit that referenced this pull request Nov 17, 2016
Summary:
Currently plain table bloom filter is required if storing metadata on file. Remove the constraint.
Closes #1525

Differential Revision: D4190977

Pulled By: siying

fbshipit-source-id: be60442
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.

3 participants