[GitHub] [druid] clintropolis commented on a change in pull request #10006: lpad and rpad functions match postrges behavior in SQL compatible mode

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

clintropolis commented on a change in pull request #10006:
URL: https://github.com/apache/druid/pull/10006#discussion_r439670297



##########
File path: core/src/main/java/org/apache/druid/java/util/common/StringUtils.java
##########
@@ -512,37 +519,47 @@ public static String lpad(String base, Integer len, String pad)
   /**
    * Returns the string right-padded with the string pad to a length of len characters.
    * If str is longer than len, the return value is shortened to len characters.
+   * This function is migrated from flink's scala function with minor refactor
+   * https://github.com/apache/flink/blob/master/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/runtime/functions/ScalarFunctions.scala
+   * - Modified to handle empty pad string.
+   * - Modified to only copy the pad string if needed (this implementation mimics lpad).
+   * - Padding of negative length return an empty string.
    *
    * @param base The base string to be padded
    * @param len  The length of padded string
    * @param pad  The pad string
    *
-   * @return the string right-padded with pad to a length of len
+   * @return the string right-padded with pad to a length of len or null if the pad is empty or the len is less than 0.
    */
-  public static String rpad(String base, Integer len, String pad)
+  @Nonnull
+  public static String rpad(@Nonnull String base, int len, @Nonnull String pad)
   {
-    if (len < 0) {
-      return null;
-    } else if (len == 0) {
+    if (len <= 0) {
       return "";
     }
 
-    char[] data = new char[len];
-
-    int pos = 0;
+    // The length of the padding needed
+    int paddingLen = Math.max(len - base.length(), 0);
 
-    // Copy the base
-    for (; pos < base.length() && pos < len; pos++) {
-      data[pos] = base.charAt(pos);
+    // short-circuit if there is no pad and we need to add a padding
+    if (paddingLen > 0 && pad.isEmpty()) {
+      return base;
     }
 
+    char[] data = new char[len];
+
     // Copy the padding
-    for (; pos < len; pos += pad.length()) {
-      for (int i = 0; i < pad.length() && i < len - pos; i++) {
-        data[pos + i] = pad.charAt(i);
+    for (int i = len - paddingLen; !pad.isEmpty() && i < len; i += pad.length()) {

Review comment:
       same comment about moving `pad.isEmpty` out

##########
File path: core/src/main/java/org/apache/druid/java/util/common/StringUtils.java
##########
@@ -472,30 +473,36 @@ public static String repeat(String s, int count)
   /**
    * Returns the string left-padded with the string pad to a length of len characters.
    * If str is longer than len, the return value is shortened to len characters.
-   * Lpad and rpad functions are migrated from flink's scala function with minor refactor
+   * This function is migrated from flink's scala function with minor refactor
    * https://github.com/apache/flink/blob/master/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/runtime/functions/ScalarFunctions.scala
+   * - Modified to handle empty pad string.
+   * - Padding of negative length return an empty string.
    *
    * @param base The base string to be padded
    * @param len  The length of padded string
    * @param pad  The pad string
    *
-   * @return the string left-padded with pad to a length of len
+   * @return the string left-padded with pad to a length of len or null if the pad is empty or the len is less than 0.
    */
-  public static String lpad(String base, Integer len, String pad)
+  @Nonnull
+  public static String lpad(@Nonnull String base, int len, @Nonnull String pad)
   {
-    if (len < 0) {
-      return null;
-    } else if (len == 0) {
+    if (len <= 0) {
       return "";
     }
 
-    char[] data = new char[len];
-
     // The length of the padding needed
     int pos = Math.max(len - base.length(), 0);
 
+    // short-circuit if there is no pad and we need to add a padding
+    if (pos > 0 && pad.isEmpty()) {
+      return base;
+    }
+
+    char[] data = new char[len];
+
     // Copy the padding
-    for (int i = 0; i < pos; i += pad.length()) {
+    for (int i = 0; !pad.isEmpty() && i < pos; i += pad.length()) {

Review comment:
       nit: should the `pad.isEmpty` check be outside of the for loop since it isn't going to change over the course of the loop?




----------------------------------------------------------------
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.

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



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org