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

Apply sample_for_compression to all block-based tables #8105

Closed
wants to merge 3 commits into from

Conversation

ajkr
Copy link
Contributor

@ajkr ajkr commented Mar 25, 2021

Previously it only applied to block-based tables generated by flush. This restriction
was undocumented and blocked a new use case. Now compression sampling
applies to all block-based tables we generate when it is enabled.

Test Plan: new unit test

Previously it only applied to files generated by flush. This restriction
was undocumented and blocked a new use case. Now compression sampling
applies to all files we generate when it is enabled.

Test Plan: new unit test
@facebook-github-bot
Copy link
Contributor

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

@ajkr ajkr changed the title Apply sample_for_compression to all files Apply sample_for_compression to all block-based tables Mar 25, 2021
Copy link
Contributor

@riversand963 riversand963 left a comment

Choose a reason for hiding this comment

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

LGTM.

return Status::OK();
}

void BlockAdd(uint64_t /* blockRawBytes */, uint64_t blockCompressedBytesFast,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: block_compressed_bytes_fast and block_compressed_bytes_slow?
To make the linter happy, we need to update the variable name in TablePropertiesCollector::BlockAdd() too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor Author

@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.

Thanks for the review!

return Status::OK();
}

void BlockAdd(uint64_t /* blockRawBytes */, uint64_t blockCompressedBytesFast,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@ajkr merged this pull request in c20a7cd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants