Skip to content

ORC-974: Remove Text Key from StringRedBlackTree#885

Closed
belugabehr wants to merge 2 commits intoapache:mainfrom
belugabehr:ORC-974
Closed

ORC-974: Remove Text Key from StringRedBlackTree#885
belugabehr wants to merge 2 commits intoapache:mainfrom
belugabehr:ORC-974

Conversation

@belugabehr
Copy link
Contributor

What changes were proposed in this pull request?

Remove the dependency on Hadoop's Text class from RedBlackTree. This has the added benefit of skipping the step where the incoming bytes are copied into the Text class.

Why are the changes needed?

Allows for removing dependency on Hadoop later; improve performance by removing copy.

How was this patch tested?

No functionality change. Using existing unit tests.

@github-actions github-actions bot added the JAVA label Aug 27, 2021
*/
private boolean add(int node, boolean fromLeft, int parent,
int grandparent, int greatGrandparent) {
private boolean add(int node, boolean fromLeft, int parent, int grandparent,
Copy link
Member

Choose a reason for hiding this comment

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

indentation?

Copy link
Member

Choose a reason for hiding this comment

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

Gentle ping, @belugabehr .

@dongjoon-hyun dongjoon-hyun changed the title ORC-974: Remove Text Key from RedBlackTree ORC-974: Remove Text Key from StringRedBlackTree Aug 28, 2021
*/
protected boolean add() {
add(root, false, NULL, NULL, NULL);
protected boolean doAdd(byte[] bytes, int offset, int length) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this renamed to avoid conflicts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. There is already a public method with the same name/signature.

newKey.set(bytes, offset, length);
return addNewKey();
// if the newKey is actually new, add it to our byteArray and store the offset & length
if (doAdd(bytes, offset, length)) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this renamed to avoid conflicts?

@dongjoon-hyun, should be here to avoid recursive calls.

Copy link
Member

Choose a reason for hiding this comment

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

Ya, that was my question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. There is already a public method with the same name/signature.

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

@github-actions github-actions bot added the Stale label Sep 7, 2022
@github-actions github-actions bot closed this Sep 8, 2022
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.

3 participants