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

refactor: Refactor CompactRow and UnsafeRow serializer and deserializer #11840

Closed

Conversation

jinchengchenghh
Copy link
Contributor

No description provided.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 12, 2024
Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit b67f20c
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/675bd2a29784840008fad02f

void clear() override {}

protected:
virtual void serializeRanges(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After this PR, we can unify serializeRanges #11206

Copy link
Contributor

@xiaoxmeng xiaoxmeng left a comment

Choose a reason for hiding this comment

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

@jinchengchenghh LGTM. Thanks for the refactor!

template <class Serializer>
class RowSerializer : public IterativeVectorSerializer {
public:
explicit RowSerializer(StreamArena* streamArena)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not directly pass pool here since we don't hold the stream arena?

const std::vector<vector_size_t>& /*rowSize*/) {
size_t offset = 0;
for (auto& range : ranges) {
for (auto i = range.begin; i < range.begin + range.size; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: s/i/row/

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@xiaoxmeng merged this pull request in aa59678.

@zuyu zuyu mentioned this pull request Dec 13, 2024
facebook-github-bot pushed a commit that referenced this pull request Dec 14, 2024
Summary:
A follow-up fix PR for #11840

Pull Request resolved: #11858

Reviewed By: spershin

Differential Revision: D67224012

Pulled By: zacw7

fbshipit-source-id: 49398f6c48aeb5876c80867c819e45fbf651c3db
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants