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

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

suneet-s commented on a change in pull request #10006:
URL: https://github.com/apache/druid/pull/10006#discussion_r437080564



##########
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:
       🤦 bad copy paste

##########
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:
       Yeah I thought I made the change because FunctionTest was passing. Fixed now.
   
   I debugged it, and saw that FunctionTest passes because it treats the empty string as null ( probably the default null handling mode in the Expr system), so the rpad function thinks the `pad` that is passed in is null, not empty.




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