Skip to content
This repository was archived by the owner on May 10, 2024. It is now read-only.

Conversation

@xhochy
Copy link
Member

@xhochy xhochy commented Jun 20, 2016

Yet we still only support PLAIN encoding. Will implement the other encodings in separate PRs to not have huge changesets.

bool dictionary_enabled_;
int64_t dictionary_pagesize_;
Encoding::type default_encoding_;
std::unordered_map<std::string, Encoding::type> encodings_;
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment explaining this member variable?

Copy link
Member

Choose a reason for hiding this comment

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

(same probably for codecs_)

Copy link
Contributor

Choose a reason for hiding this comment

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

Or perhaps refactor these pairs of variables into instances of PerColumnProperties<Encoding::type/Compression::type> to avoid duplicating comments.

@wesm
Copy link
Member

wesm commented Jun 21, 2016

Looks good outside minor comment. We can spare implementing the Parquet 2.0 encodings for now.

@wesm
Copy link
Member

wesm commented Jun 21, 2016

+1, will merge on green build

@xhochy
Copy link
Member Author

xhochy commented Jun 22, 2016

Build is green ;)

@asfgit asfgit closed this in 53475c7 Jun 22, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants