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

ORC-747: Abstract Dictionary interface and refactoring #637

Closed
wants to merge 6 commits into from

Conversation

autumnust
Copy link
Contributor

What changes were proposed in this pull request?

This PR contains the very first part to make the implementation of dictionary used for encoding pluggable, by introducing an interface Dictionary. The subsequent PR will add HashTable-based Dictionary implementation and compare with the existing RB-tree based dictionary.

For reviewers:

  • Please comment if reflection-based initialization will be preferred or not since I didn't found much of that within the code base.
  • The Dictionary class isn't added with generic type declaration (e.g. we could use a generic type T to replace Text). Please let me know if that's necessary to add.

Why are the changes needed?

How was this patch tested?

Since this is mostly a refactoring, it is just passing all existing unit tests.

@dongjoon-hyun dongjoon-hyun changed the title [ORC-50] Abstract Dictionary interface and refactoring around that ORC-50: Abstract Dictionary interface and refactoring around that Jan 27, 2021
@dongjoon-hyun
Copy link
Member

Thank you for making a PR, @autumnust . It seems that many failure. Could you test it locally first?

Error:  Tests run: 1146, Failures: 0, Errors: 48, Skipped: 1

@dongjoon-hyun dongjoon-hyun changed the title ORC-50: Abstract Dictionary interface and refactoring around that ORC-747: Abstract Dictionary interface and refactoring Jan 27, 2021
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

This PR is partial for ORC-50. So, I create a new JIRA for this PR. After we finish all implementation, we can resolve ORC-50 as an umbrella JIRA.

@dongjoon-hyun
Copy link
Member

Gentle ping, @autumnust .

@autumnust
Copy link
Contributor Author

Gentle ping, @autumnust .

Sorry I got preempted by some other thing last week. I will get back to fixing the failure in Github Action ASAP. Thanks @dongjoon-hyun for your review in first round.

@omalley omalley closed this in 8c5814b Feb 23, 2021
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.

4 participants