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 15:52:04 UTC
[GitHub] [druid] suneet-s opened a new pull request #10006: lpad and rpad functions deal with empty pad
suneet-s opened a new pull request #10006:
URL: https://github.com/apache/druid/pull/10006
### Description
Return null if the pad string used by the `lpad` and `rpad` functions is
an empty string
<hr>
This PR has:
- [x] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [x] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/licenses.yaml)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [x] added unit tests or modified existing tests to cover new code paths.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist above are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
<hr>
##### Key changed/added classes in this PR
* `MyFoo`
* `OurBar`
* `TheirBaz`
----------------------------------------------------------------
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
[GitHub] [druid] suneet-s commented on a change in pull request #10006: lpad and rpad functions deal with empty pad
Posted by 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
[GitHub] [druid] suneet-s commented on a change in pull request #10006: lpad and rpad functions match postrges behavior in SQL compatible mode
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10006:
URL: https://github.com/apache/druid/pull/10006#discussion_r440298769
##########
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:
Done
##########
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:
Done
----------------------------------------------------------------
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
[GitHub] [druid] suneet-s commented on a change in pull request #10006: lpad and rpad functions deal with empty pad
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10006:
URL: https://github.com/apache/druid/pull/10006#discussion_r438311543
##########
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:
I was going to remove the annotations, but then I saw that we don't have `@EverythingIsNonnullByDefault` on this package. Since this is in core, I'm a little more paranoid about adding the annotation to this package. 😅
I'd like to keep this as is for now.
----------------------------------------------------------------
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
[GitHub] [druid] clintropolis commented on a change in pull request #10006: lpad and rpad functions match postrges behavior in SQL compatible mode
Posted by 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
[GitHub] [druid] clintropolis commented on a change in pull request #10006: lpad and rpad functions deal with empty pad
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [druid] suneet-s commented on a change in pull request #10006: lpad and rpad functions deal with empty pad
Posted by GitBox <gi...@apache.org>.
suneet-s commented on a change in pull request #10006:
URL: https://github.com/apache/druid/pull/10006#discussion_r438311543
##########
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:
I was going to remove the annotations, but then I saw that we don't have `@EverythingIsNonnullByDefault` on this package. Since this is in core, I'm a little more paranoid about adding the annotation to this package. I'd like to keep this as is for now.
----------------------------------------------------------------
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
[GitHub] [druid] ccaominh commented on a change in pull request #10006: lpad and rpad functions deal with empty pad
Posted by GitBox <gi...@apache.org>.
ccaominh commented on a change in pull request #10006:
URL: https://github.com/apache/druid/pull/10006#discussion_r438493609
##########
File path: core/src/test/java/org/apache/druid/java/util/common/StringUtilsTest.java
##########
@@ -211,39 +204,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);
Review comment:
Looks like postgres returns an empty string for this case (but mysql returns `null`)
##########
File path: core/src/test/java/org/apache/druid/java/util/common/StringUtilsTest.java
##########
@@ -211,39 +204,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.assertEquals("abc", lpad);
+
+ lpad = StringUtils.lpad("abc", 1, "");
+ Assert.assertEquals("a", 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);
Review comment:
Similar comment as `lpad`
----------------------------------------------------------------
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
[GitHub] [druid] suneet-s merged pull request #10006: lpad and rpad functions match postrges behavior in SQL compatible mode
Posted by GitBox <gi...@apache.org>.
suneet-s merged pull request #10006:
URL: https://github.com/apache/druid/pull/10006
----------------------------------------------------------------
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
[GitHub] [druid] suneet-s commented on pull request #10006: lpad and rpad functions deal with empty pad
Posted by GitBox <gi...@apache.org>.
suneet-s commented on pull request #10006:
URL: https://github.com/apache/druid/pull/10006#issuecomment-642355568
Tagged Release Notes because this PR changes the behavior of these functions to be more in line with PostgreSQL
----------------------------------------------------------------
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
[GitHub] [druid] ccaominh commented on a change in pull request #10006: lpad and rpad functions deal with empty pad
Posted by 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