[GitHub] [orc] belugabehr commented on a change in pull request #885: ORC-974: Remove Text Key from StringRedBlackTree

[GitBox <gi...@apache.org>]

belugabehr commented on a change in pull request #885:
URL: https://github.com/apache/orc/pull/885#discussion_r700241191



##########
File path: java/core/src/java/org/apache/orc/impl/StringRedBlackTree.java
##########
@@ -29,49 +30,42 @@
 public class StringRedBlackTree extends RedBlackTree implements Dictionary {
   private final DynamicByteArray byteArray = new DynamicByteArray();
   private final DynamicIntArray keyOffsets;
-  private final Text newKey = new Text();
 
   public StringRedBlackTree(int initialCapacity) {
     super(initialCapacity);
     keyOffsets = new DynamicIntArray(initialCapacity);
   }
 
+  @Deprecated
   public int add(String value) {
-    newKey.set(value);
-    return addNewKey();
-  }
-
-  private int addNewKey() {
-    // if the newKey is actually new, add it to our byteArray and store the offset & length
-    if (add()) {
-      int len = newKey.getLength();
-      keyOffsets.add(byteArray.add(newKey.getBytes(), 0, len));
-    }
-    return lastAdd;
+    byte[] b = value.getBytes(StandardCharsets.UTF_8);
+    return add(b, 0, b.length);
   }
 
+  @Deprecated
   public int add(Text value) {
-    newKey.set(value);
-    return addNewKey();
+    return add(value.getBytes(), 0, value.getLength());
   }
 
   @Override
   public int add(byte[] bytes, int offset, int length) {
-    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)) {

Review comment:
       Yes.  There is already a `public` method with the same name/signature.

##########
File path: java/core/src/java/org/apache/orc/impl/RedBlackTree.java
##########
@@ -271,10 +271,14 @@ private boolean add(int node, boolean fromLeft, int parent,
 
   /**
    * Add the new key to the tree.
+   *
+   * @param bytes
+   * @param offset
+   * @param length
    * @return true if the element is a new one.
    */
-  protected boolean add() {
-    add(root, false, NULL, NULL, NULL);
+  protected boolean doAdd(byte[] bytes, int offset, int length) {

Review comment:
       Yes.  There is already a `public` method with the same name/signature.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@orc.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org