Support LPAD and RPAD sql function #7388

xueyumusic · 2019-03-31T15:00:29Z

LPAD and RPAD are sql functions supported by oracle and hive etc. This PR tries to add them in druid.
The syntax is LPAD(base, len, pad).
It adds padding characters to the left or right side of a base string up to a given length. The default padding character is a space. If the string's length is greater than the required length, it will be trimmed (excess characters will be removed).

clintropolis

overall lgtm 👍

clintropolis · 2019-04-02T19:37:18Z

core/src/main/java/org/apache/druid/math/expr/Function.java

+
+      String base = NullHandling.nullToEmptyIfNeeded(args.get(0).eval(bindings).asString());
+      int len = args.get(1).eval(bindings).asInt();
+      String pad = NullHandling.nullToEmptyIfNeeded(args.get(2).eval(bindings).asString());


Does this make sense for the pad? Seems like it should maybe be an error if it's null?

@clintropolis Thanks for review. When base or pad is null, it does not make sense to do these.

clintropolis · 2019-04-02T19:39:30Z

core/src/main/java/org/apache/druid/math/expr/Function.java

+        throw new IAE("Function[%s] needs 3 arguments", name());
+      }
+
+      String base = NullHandling.nullToEmptyIfNeeded(args.get(0).eval(bindings).asString());


Does it make sense to pad empty values when sql compatible null handling is not enabled?

clintropolis · 2019-04-02T19:42:17Z

core/src/main/java/org/apache/druid/java/util/common/StringUtils.java

+   * @param pad The pad string
+   * @return the string left-padded with pad to a lenght of len
+   */
+  public static String lpad(String base, Integer len, String pad)


I believe guava and commons-lang also have string padding functions, how do these compare to those implementations? (Not a suggestion to use either of those, just curious about the choice)

@clintropolis I looked at guava and commons-lang3 StringUtils, it has different behavior from expected for length less than base string, such as lpad("bat", 1, "yz") expected b but commons-lang3 gave bat. And guava only pad single char. This method is migrated from Flink's scala method

Thanks for the explanation, seems reasonable 👍

Could you add this link in the javadoc for this method?

clintropolis · 2019-04-09T20:56:38Z

core/src/main/java/org/apache/druid/java/util/common/StringUtils.java

+   * @param pad The pad string
+   * @return the string left-padded with pad to a lenght of len
+   */
+  public static String lpad(String base, Integer len, String pad)


Could you add this link in the javadoc for this method?

clintropolis · 2019-04-09T20:57:04Z

core/src/main/java/org/apache/druid/java/util/common/StringUtils.java

+   * @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 lenght of len


typo lenght -> length

clintropolis · 2019-04-09T20:57:46Z

core/src/main/java/org/apache/druid/java/util/common/StringUtils.java

+   * @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 lenght of len


same lenght typo

clintropolis · 2019-04-09T21:15:48Z

core/src/main/java/org/apache/druid/math/expr/Function.java

+      int len = args.get(1).eval(bindings).asInt();
+      String pad = args.get(2).eval(bindings).asString();
+
+      if (base == null || pad == null) {


I feel like pad being null should be an error case instead of silently returning null, but what do Oracle and Hive do in this case?

Hmm, interesting, did a bit of searching and from the docs I've found, it seems like they have different behavior, with Oracle having a default pad value of a single blank character, and Hive returning null as you are doing here. If this is true, I'm not actually sure what is best thing to do here...

Hi, @clintropolis , Postgres/Orcale has two func signatures which are two params or three params. When only having two params such as lpad('abc',5) it will pad with default blank char.

For three params, I tested postgres, oracle and hive when padding or base is null.
Postgres and hive:

select lpad('b',5,null) is null; ##result is true select lpad(null,5,'x') is null; ##result is true

Oracle

create table test1(a int); insert into test1(a) values(5); select count(1) from test1 where lpad('abc',5,null) is null;# result is 1 select count(1) from test1 where lpad(null,5,'x') is null; #result is 1 select count(1) from test1 where lpad('abc',5) is null; # result is 0

It looks the results are all null when base or pad is null and the same

Thanks for testing! null seems like the thing to do then 👍

justinborromeo · 2019-04-09T21:37:41Z

core/src/main/java/org/apache/druid/java/util/common/StringUtils.java

+    // The length of the padding needed
+    int pos = Math.max(len - base.length(), 0);
+
+    // Copy the padding


For the sake of conciseness, can this be shortened to use for loops?

// Copy the padding for (int i = 0; i < pos; i += pad.length()) { for (int j = 0; j < pad.length() && j < pos - i; j++) { data[i + j] = padChars[j]; } } // Copy the base for (int i = 0; pos + i < len && i < base.length(); i++) { data[pos + i] = baseChars[i]; }

justinborromeo · 2019-04-09T21:40:35Z

core/src/main/java/org/apache/druid/java/util/common/StringUtils.java

+    }
+
+    char[] data = new char[len];
+    char[] baseChars = base.toCharArray();


Is there a reason you copy base and pad to their own char arrays instead of just calling charAt(index) on the original strings?

xueyumusic · 2019-04-10T12:15:54Z

Hi, @clintropolis @justinborromeo Thanks a lot for review! I updated the codes according to your comments.

justinborromeo

Overall lgtm - some minor formatting and comment nits

justinborromeo · 2019-04-10T17:18:48Z

sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java

+  @Test
+  public void testSelectPadFamily() throws Exception
+  {
+    // TRIM has some whacky parsing. Make sure the different forms work.


This comment doesn't seem to apply to this test.

justinborromeo · 2019-04-10T17:24:08Z

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/LPadOperatorConversion.java

+        }
+    );
+  }
+}


Nit - please include a newline at the end of the file

Hi, @justinborromeo , thanks for review. It seems that this file is ended as 0x7d 0x0a which are }\n. From raw file it looks having a blank ending line. Most other java files seems the same. I an not sure whether I understood correctly..Thank you

justinborromeo · 2019-04-10T17:25:00Z

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/RPadOperatorConversion.java

+        }
+    );
+  }
+}


Nit - please include a newline at the end of the file

justinborromeo · 2019-04-10T21:32:51Z

@xueyumusic Could you also add the LPAD and RPAD functions to the SQL doc?

xueyumusic · 2019-04-11T05:51:17Z

Thanks for review! @justinborromeo , updated according to your comments

clintropolis · 2019-04-11T20:15:43Z

docs/content/misc/math-expr.md

@@ -85,6 +85,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, left-padded to a specified length with the specified characters; or, when the string to be padded is longer than the length specified after padding, only that portion of the expression that fits into the specified length.


The documentation for these functions appears to be a near word for word copy of the Oracle documentation of these functions, with 'expression' replaced with 'string' in most cases. I think this is risky, https://www.oracle.com/legal/copyright.html indicates that both code and documentation is covered, and I think these descriptions need rewritten.

Maybe something like

lpad(expr, length, chars) returns a string of length from expr left-padded with chars. If either expr or chars are null, the result will be null.

Thanks a lot for reminding, @clintropolis . I totally agree with you and updated the doc.

clintropolis

👍

clintropolis added the Area - SQL label Apr 2, 2019

clintropolis reviewed Apr 2, 2019

View reviewed changes

xueyumusic force-pushed the sql_pad branch from a156af8 to a063780 Compare April 4, 2019 15:11

clintropolis reviewed Apr 9, 2019

View reviewed changes

justinborromeo reviewed Apr 9, 2019

View reviewed changes

xueyumusic added 3 commits April 10, 2019 13:52

lpad and rpad sql function

a8f2419

feedback address

a6f7ab4

feedback address

7d3474a

xueyumusic force-pushed the sql_pad branch from a063780 to 7d3474a Compare April 10, 2019 10:06

justinborromeo approved these changes Apr 10, 2019

View reviewed changes

add doc and format

68ff3be

clintropolis requested changes Apr 11, 2019

View reviewed changes

update docs

d2af469

clintropolis approved these changes Apr 14, 2019

View reviewed changes

clintropolis merged commit 2c8a71f into apache:master Apr 22, 2019

jon-wei added the Release Notes label May 15, 2019

jihoonson added this to the 0.15.0 milestone May 15, 2019

jihoonson mentioned this pull request Jun 8, 2019

0.15.0-incubating release notes #7854

Closed

jihoonson mentioned this pull request Aug 14, 2019

add copyright info back to NOTICE and NOTICE.BINARY #8298

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support LPAD and RPAD sql function #7388

Support LPAD and RPAD sql function #7388

xueyumusic commented Mar 31, 2019

clintropolis left a comment

clintropolis Apr 2, 2019

xueyumusic Apr 4, 2019

clintropolis Apr 2, 2019

clintropolis Apr 2, 2019

xueyumusic Apr 4, 2019

clintropolis Apr 9, 2019

clintropolis Apr 9, 2019

clintropolis Apr 9, 2019

clintropolis Apr 9, 2019

clintropolis Apr 9, 2019

clintropolis Apr 9, 2019

clintropolis Apr 9, 2019

xueyumusic Apr 10, 2019

clintropolis Apr 10, 2019

justinborromeo Apr 9, 2019

justinborromeo Apr 9, 2019

xueyumusic commented Apr 10, 2019

justinborromeo left a comment

justinborromeo Apr 10, 2019

justinborromeo Apr 10, 2019

xueyumusic Apr 11, 2019

justinborromeo Apr 10, 2019

justinborromeo commented Apr 10, 2019

xueyumusic commented Apr 11, 2019

clintropolis Apr 11, 2019

xueyumusic Apr 12, 2019

clintropolis left a comment

Support LPAD and RPAD sql function #7388

Support LPAD and RPAD sql function #7388

Conversation

xueyumusic commented Mar 31, 2019

clintropolis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xueyumusic commented Apr 10, 2019

justinborromeo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinborromeo commented Apr 10, 2019

xueyumusic commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintropolis left a comment

Choose a reason for hiding this comment