You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/06/09 16:05:38 UTC
[GitHub] [druid] clintropolis commented on a change in pull request #10006: lpad and rpad functions deal with empty pad
clintropolis commented on a change in pull request #10006:
URL: https://github.com/apache/druid/pull/10006#discussion_r437096339
##########
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:
upvote for preferring postgres behavior to mysql
##########
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)
Review comment:
Afaik we don't traditionally annotate things explicitly with `@Nonnull`, in preference of using `@Nullable` and `@EverythingIsNonnullByDefault` in package.info, however I guess we haven't gone through and set that everywhere. However, to be more consistent with other places in the code maybe that would be better?
##########
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:
Actually, looking closer this will need changed here too https://github.com/apache/druid/blob/master/core/src/main/java/org/apache/druid/math/expr/Function.java#L1917 since it is treating the value as null if the pad is null. If we fix the issue there and check if pad is null or empty we can avoid entering the `StringUtils` function entirely (though we should still fix the `StringUtils` method)
##########
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)
Review comment:
To add to this, there are plenty of existing `@Nonnull` annotations in the code, so this change isn't really required, mostly just expressing my preference to keep things from getting too verbose 😅
##########
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:
Actually, looking closer this will need changed here too https://github.com/apache/druid/blob/master/core/src/main/java/org/apache/druid/math/expr/Function.java#L1917 since it is treating the value as null if the pad is null. If we fix the issue there and check if pad is null or empty we can avoid entering the `StringUtils` function entirely (though we should still fix the `StringUtils` method)
If we change the behavior with null input we should also tag this with release notes label.
----------------------------------------------------------------
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