Skip to content

lpad and rpad functions match postrges behavior in SQL compatible mode #10006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
suneet-s merged 9 commits into from
Jun 15, 2020
Merged

lpad and rpad functions match postrges behavior in SQL compatible mode #10006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a91399c
lpad and rpad functions deal with empty pad
suneet-s Jun 9, 2020
170ec78
Fix rpad
suneet-s Jun 9, 2020
778d47c
Merge remote-tracking branch 'upstream/master' into lpad
suneet-s Jun 9, 2020
98889d8
Merge remote-tracking branch 'upstream/master' into lpad
suneet-s Jun 10, 2020
62909d9
Merge remote-tracking branch 'upstream/master' into lpad
suneet-s Jun 10, 2020
b3857fe
Match PostgreSQL behavior in SQL compliant null handling mode
suneet-s Jun 10, 2020
ed26264
Match PostgreSQL behavior for pad -ve len
suneet-s Jun 11, 2020
e27ccb6
Merge remote-tracking branch 'upstream/master' into lpad
suneet-s Jun 15, 2020
217443a
address review comments
suneet-s Jun 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 40 additions & 22 deletions core/src/main/java/org/apache/druid/java/util/common/StringUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import com.google.common.base.Strings;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import java.io.UnsupportedEncodingException;
import java.net.URLDecoder;
Expand Down Expand Up @@ -472,28 +473,34 @@ 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)
Copy link
Copy Markdown
Member

@clintropolis clintropolis Jun 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@clintropolis clintropolis Jun 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😅

Copy link
Copy Markdown
Contributor Author

@suneet-s suneet-s Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
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 j = 0; j < pad.length() && j < pos - i; j++) {
Expand All @@ -512,37 +519,48 @@ 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; i < len; i += pad.length()) {
for (int j = 0; j < pad.length() && i + j < data.length; j++) {
data[i + j] = pad.charAt(j);
}
}

// Copy the base
for (int i = 0; i < len && i < base.length(); i++) {
data[i] = base.charAt(i);
}

return new String(data);
}

Expand Down
59 changes: 32 additions & 27 deletions core/src/test/java/org/apache/druid/java/util/common/StringUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,13 +133,6 @@ public void fromUtf8ByteBufferDirect()
Assert.assertEquals("abcd", StringUtils.fromUtf8(bytes));
}

@Test
public void testCharsetShowsUpAsDeprecated()
{
// Not actually a runnable test, just checking the IDE
Assert.assertNotNull(StringUtils.UTF8_CHARSET);
}

@SuppressWarnings("MalformedFormatString")
@Test
public void testNonStrictFormat()
Expand Down Expand Up @@ -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.assertEquals("", 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.assertEquals("", rpad);

String s4 = StringUtils.rpad("abc", 0, "de");
Assert.assertEquals(s4, "");
rpad = StringUtils.rpad("abc", 10, "");
Assert.assertEquals("abc", rpad);

String s5 = StringUtils.rpad("abc", -1, "de");
Assert.assertEquals(s5, null);
rpad = StringUtils.rpad("abc", 1, "");
Assert.assertEquals("a", rpad);
}

@Test
Expand Down
39 changes: 33 additions & 6 deletions core/src/test/java/org/apache/druid/math/expr/FunctionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,23 @@ public void testLpad()
assertExpr("lpad(x, 5, 'ab')", "abfoo");
assertExpr("lpad(x, 4, 'ab')", "afoo");
assertExpr("lpad(x, 2, 'ab')", "fo");
assertArrayExpr("lpad(x, 0, 'ab')", null);
assertArrayExpr("lpad(x, 5, null)", null);
assertArrayExpr("lpad(null, 5, x)", null);
assertExpr("lpad(x, -1, 'ab')", NullHandling.replaceWithDefault() ? null : "");
assertExpr("lpad(null, 5, 'ab')", null);
assertExpr("lpad(x, 2, '')", NullHandling.replaceWithDefault() ? null : "fo");
assertExpr("lpad(x, 6, '')", NullHandling.replaceWithDefault() ? null : "foo");
assertExpr("lpad('', 3, '*')", NullHandling.replaceWithDefault() ? null : "***");
assertExpr("lpad(x, 2, null)", null);
assertExpr("lpad(a, 4, '*')", "[foo");
assertExpr("lpad(a, 2, '*')", "[f");
assertExpr("lpad(a, 2, '')", NullHandling.replaceWithDefault() ? null : "[f");
assertExpr("lpad(b, 4, '*')", "[1, ");
assertExpr("lpad(b, 2, '')", NullHandling.replaceWithDefault() ? null : "[1");
assertExpr("lpad(b, 2, null)", null);
assertExpr("lpad(x, 5, x)", "fofoo");
assertExpr("lpad(x, 5, y)", "22foo");
assertExpr("lpad(x, 5, z)", "3.foo");
assertExpr("lpad(y, 5, x)", "foof2");
assertExpr("lpad(z, 5, y)", "223.1");
}

@Test
Expand All @@ -157,9 +171,22 @@ public void testRpad()
assertExpr("rpad(x, 5, 'ab')", "fooab");
assertExpr("rpad(x, 4, 'ab')", "fooa");
assertExpr("rpad(x, 2, 'ab')", "fo");
assertArrayExpr("rpad(x, 0, 'ab')", null);
assertArrayExpr("rpad(x, 5, null)", null);
assertArrayExpr("rpad(null, 5, x)", null);
assertExpr("rpad(x, -1, 'ab')", NullHandling.replaceWithDefault() ? null : "");
assertExpr("rpad(null, 5, 'ab')", null);
assertExpr("rpad(x, 2, '')", NullHandling.replaceWithDefault() ? null : "fo");
assertExpr("rpad(x, 6, '')", NullHandling.replaceWithDefault() ? null : "foo");
assertExpr("rpad('', 3, '*')", NullHandling.replaceWithDefault() ? null : "***");
assertExpr("rpad(x, 2, null)", null);
assertExpr("rpad(a, 2, '*')", "[f");
assertExpr("rpad(a, 2, '')", NullHandling.replaceWithDefault() ? null : "[f");
assertExpr("rpad(b, 4, '*')", "[1, ");
assertExpr("rpad(b, 2, '')", NullHandling.replaceWithDefault() ? null : "[1");
assertExpr("rpad(b, 2, null)", null);
assertExpr("rpad(x, 5, x)", "foofo");
assertExpr("rpad(x, 5, y)", "foo22");
assertExpr("rpad(x, 5, z)", "foo3.");
assertExpr("rpad(y, 5, x)", "2foof");
assertExpr("rpad(z, 5, y)", "3.122");
}

@Test
Expand Down
4 changes: 2 additions & 2 deletions docs/misc/math-expr.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ The following built-in functions are available.
|upper|upper(expr) converts a string to uppercase|
|reverse|reverse(expr) reverses a string|
|repeat|repeat(expr, N) repeats a string N times|
|lpad|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|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|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` or `chars` is null. If `chars` is an empty string, no padding is added, however `expr` may be trimmed if necessary.|
|rpad|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`. The result will be null if either `expr` or `chars` is null. If `chars` is an empty string, no padding is added, however `expr` may be trimmed if necessary.|

## Time functions

Expand Down
4 changes: 2 additions & 2 deletions docs/querying/sql.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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` or `chars` is null. If `chars` is an empty string, no padding is added, however `expr` may be trimmed if necessary.|
|`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`. The result will be null if either `expr` or `chars` is null. If `chars` is an empty string, no padding is added, however `expr` may be trimmed if necessary.|


### Time functions
Expand Down