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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
