[GitHub] [druid] ccaominh commented on a change in pull request #10006: lpad and rpad functions deal with empty pad

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

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



##########
File path: core/src/main/java/org/apache/druid/java/util/common/StringUtils.java
##########
@@ -481,9 +482,10 @@ public static String repeat(String s, int count)
    *
    * @return the string left-padded with pad to a length of len
    */
-  public static String lpad(String base, Integer len, String pad)
+  @Nullable
+  public static String lpad(@Nonnull String base, int len, @Nonnull String pad)
   {
-    if (len < 0) {
+    if (len < 0 || pad.isEmpty()) {

Review comment:
       Doesn't `rpad()` need a similar change?

##########
File path: core/src/test/java/org/apache/druid/java/util/common/StringUtilsTest.java
##########
@@ -211,39 +211,51 @@ public void testRepeat()
   @Test
   public void testLpad()
   {
-    String s1 = StringUtils.lpad("abc", 7, "de");
-    Assert.assertEquals(s1, "dedeabc");
+    String lpad = StringUtils.lpad("abc", 7, "de");
+    Assert.assertEquals("dedeabc", lpad);
 
-    String s2 = StringUtils.lpad("abc", 6, "de");
-    Assert.assertEquals(s2, "dedabc");
+    lpad = StringUtils.lpad("abc", 6, "de");
+    Assert.assertEquals("dedabc", lpad);
 
-    String s3 = StringUtils.lpad("abc", 2, "de");
-    Assert.assertEquals(s3, "ab");
+    lpad = StringUtils.lpad("abc", 2, "de");
+    Assert.assertEquals("ab", lpad);
 
-    String s4 = StringUtils.lpad("abc", 0, "de");
-    Assert.assertEquals(s4, "");
+    lpad = StringUtils.lpad("abc", 0, "de");
+    Assert.assertEquals("", lpad);
 
-    String s5 = StringUtils.lpad("abc", -1, "de");
-    Assert.assertEquals(s5, null);
+    lpad = StringUtils.lpad("abc", -1, "de");
+    Assert.assertNull(lpad);
+
+    lpad = StringUtils.lpad("abc", 10, "");
+    Assert.assertNull(lpad);
+
+    lpad = StringUtils.lpad("abc", 1, "");
+    Assert.assertNull(lpad);
   }
 
   @Test
   public void testRpad()
   {
-    String s1 = StringUtils.rpad("abc", 7, "de");
-    Assert.assertEquals(s1, "abcdede");
+    String rpad = StringUtils.rpad("abc", 7, "de");
+    Assert.assertEquals("abcdede", rpad);
+
+    rpad = StringUtils.rpad("abc", 6, "de");
+    Assert.assertEquals("abcded", rpad);
+
+    rpad = StringUtils.rpad("abc", 2, "de");
+    Assert.assertEquals("ab", rpad);
 
-    String s2 = StringUtils.rpad("abc", 6, "de");
-    Assert.assertEquals(s2, "abcded");
+    rpad = StringUtils.rpad("abc", 0, "de");
+    Assert.assertEquals("", rpad);
 
-    String s3 = StringUtils.rpad("abc", 2, "de");
-    Assert.assertEquals(s3, "ab");
+    rpad = StringUtils.rpad("abc", -1, "de");
+    Assert.assertNull(rpad);
 
-    String s4 = StringUtils.rpad("abc", 0, "de");
-    Assert.assertEquals(s4, "");
+    rpad = StringUtils.lpad("abc", 10, "");

Review comment:
       Shouldn't this call `rpad()`? (same below)

##########
File path: docs/querying/sql.md
##########
@@ -337,8 +337,8 @@ String functions accept strings, and return a type appropriate to the function.
 |`UPPER(expr)`|Returns expr in all uppercase.|
 |`REVERSE(expr)`|Reverses expr.|
 |`REPEAT(expr, [N])`|Repeats expr N times|
-|`LPAD(expr, length[, chars])`|Returns a string of "length" from "expr" left-padded with "chars". If "length" is shorter than the length of "expr", the result is "expr" which is truncated to "length". If either "expr" or "chars" are null, the result will be null.|
-|`RPAD(expr, length[, chars])`|Returns a string of "length" from "expr" right-padded with "chars". If "length" is shorter than the length of "expr", the result is "expr" which is truncated to "length". If either "expr" or "chars" are null, the result will be null.|
+|`LPAD(expr, length[, chars])`|Returns a string of `length` from `expr` left-padded with `chars`. If `length` is shorter than the length of `expr`, the result is `expr` which is truncated to `length`. The result will be null if either `expr` is null, or `chars` is either null or empty.|

Review comment:
       Sorry I missed this before, but I tried `lpad` and `rpad` with mysql and postgres since they aren't standard functions. If the padding is an empty string, mysql returns `null` whereas postgres returns the original string. In the past, we've matched the behavior of postgres (e.g., https://github.com/apache/druid/pull/9488#discussion_r391092673), so it'd be good to match the postgres behavior for `lpad` and `rpad` as well.




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