[FLINK-10145][table] Add replace supported in TableAPI and SQL #6576

Guibo-Pan · 2018-08-18T17:32:00Z

What is the purpose of the change

This pull request add replace supported in TableAPI and SQL

Brief change log

Add replace supported in TableAPI and SQL

Verifying this change

This change is already covered by existing tests, such as ScalarFunctionsTest#testReplace.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (docs)

Guibo-Pan · 2018-08-19T04:06:45Z

cc @yanghua @tillrohrmann a small feature~

yanghua

Hi @Guibo-Pan thanks for your contribution, I left some comment you can consider.

yanghua · 2018-08-19T05:23:49Z

docs/dev/table/functions.md

+      </td>
+      <td>
+        <p>Returns a new string from <i>STRING</i> replaced <i>STRING1</i>(non-overlapping) with <i>STRING2</i>.</p>
+        <p>E.g., <code>'hello world'.replace('world', 'flink')</code> returns 'hello flink'; <code>'ababab'.replace('abab', 'z')</code> returns "zab"</p>


replace all '' to ""

yanghua · 2018-08-19T05:23:54Z

docs/dev/table/functions.md

+      </td>
+      <td>
+        <p>Returns a new string from <i>STRING</i> replaced <i>STRING1</i>(non-overlapping) with <i>STRING2</i>.</p>
+        <p>E.g., <code>'hello world'.replace('world', 'flink')</code> returns 'hello flink'; <code>'ababab'.replace('abab', 'z')</code> returns "zab"</p>


replace all '' to ""

yanghua · 2018-08-19T05:24:26Z

flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/scala/expressionDsl.scala

+    * Create a new string of the given string with non-overlapping occurrences
+    * of given search replaced with replacement.
+    *
+    * @param search


remove @param and @return to match the style of other methods

yanghua · 2018-08-19T05:24:34Z

...raries/flink-table/src/main/scala/org/apache/flink/table/expressions/stringExpressions.scala

@@ -410,3 +410,27 @@ case class ToBase64(child: Expression) extends UnaryExpression with InputTypeSpe
  override def toString: String = s"($child).toBase64"

 }
+
+/**
+  * Returns the string `str` with all non-overlapping occurrences


remove all the "``" , just describe the function

yanghua · 2018-08-19T05:25:45Z

...ries/flink-table/src/test/scala/org/apache/flink/table/expressions/ScalarFunctionsTest.scala

@@ -94,6 +94,21 @@ class ScalarFunctionsTest extends ScalarTypesTestBase {
      "his is a test String.")
  }

+  @Test
+  def testReplace(): Unit = {


add more test for invalid input such : empty string, null or a string is not available of the original string

yanghua · 2018-08-19T05:25:50Z

...raries/flink-table/src/test/scala/org/apache/flink/table/expressions/SqlExpressionTest.scala

@@ -149,6 +149,9 @@ class SqlExpressionTest extends ExpressionTestBase {
    testSqlApi("RPAD('hi',4,'??')", "hi??")
    testSqlApi("FROM_BASE64('aGVsbG8gd29ybGQ=')", "hello world")
    testSqlApi("TO_BASE64('hello world')", "aGVsbG8gd29ybGQ=")
+    testSqlApi("REPLACE('hello world', 'world', 'flink')", "hello flink")
+    testSqlApi("REPLACE('ababab', 'abab', 'Z')", "Zab")
+    testSqlApi("REPLACE('ababab', 'a', '')", "bbb")


It seems this test file is a show case, so just add one example .

Guibo-Pan · 2018-08-19T17:58:06Z

@yanghua thanks for your suggestion, I've made an update.

xccui

Thanks for this PR @Guibo-Pan. I left some comments, mainly about the style and the code duplication.

Thanks, Xingcan

xccui · 2018-08-21T01:33:50Z

docs/dev/table/functions.md

+      </td>
+      <td>
+        <p>Returns a new string from <i>string</i> replaced <i>search</i>(non-overlapping) with <i>replacement</i>.</p>
+        <p>E.g., <code>REPLACE("hello world", "world", "flink")</code> returns "hello flink"; <code>REPLACE("ababab", "abab", "z")</code> returns "zab"</p>


Use single quotes for strings in SQL an Java <code> contexts.

xccui · 2018-08-21T01:37:32Z

docs/dev/table/functions.md

+    <tr>
+      <td>
+        {% highlight text %}
+REPLACE(string, search, replacement)


For style consistency, just use string1, string2 and string3 here.

xccui · 2018-08-21T01:41:50Z

docs/dev/table/functions.md

+{% endhighlight %}
+      </td>
+      <td>
+        <p>Returns a new string from <i>string</i> replaced <i>search</i>(non-overlapping) with <i>replacement</i>.</p>


Returns a new string which replaces string2 with string3 (non-overlapping) from string1. Please update the Java and Scala documentation correspondingly.

xccui · 2018-08-21T01:45:46Z

docs/dev/table/functions.md

+      </td>
+      <td>
+        <p>Returns a new string from <i>STRING</i> replaced <i>STRING1</i>(non-overlapping) with <i>STRING2</i>.</p>
+        <p>E.g., <code>"hello world".replace("world", "flink")</code> returns "hello flink"; <code>"ababab".replace("abab", "z")</code> returns "zab"</p>


Don't forget the period at the end of a sentence.

xccui · 2018-08-21T01:47:22Z

flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/scala/expressionDsl.scala

@@ -459,6 +459,15 @@ trait ImplicitExpressionOperations {
    }
  }

+  /**
+    * Create a new string of the given string with non-overlapping occurrences


Use singular for docs here (Create -> Creates).

xccui · 2018-08-21T01:49:38Z

flink-libraries/flink-table/src/main/scala/org/apache/flink/table/api/scala/expressionDsl.scala

+    * Create a new string of the given string with non-overlapping occurrences
+    * of given search replaced with replacement.
+    */
+  def replace(search: Expression,


The linebreak-style should be consistent with others (check the overlap method for an example).

xccui · 2018-08-21T02:01:02Z

...ies/flink-table/src/main/scala/org/apache/flink/table/functions/sql/ScalarSqlFunctions.scala

@@ -67,6 +67,14 @@ object ScalarSqlFunctions {
    OperandTypes.ONE_OR_MORE,
    SqlFunctionCategory.STRING)

+  val REPLACE = new SqlFunction(


The REPLACE SqlFunction has been provided in org.apache.calcite.sql.fun.SqlStdOperatorTable.

I use SqlStdOperatorTable.REPLACE at first, however that REPLACE function can not handle null params, so I create a new function which refer to SqlStdOperatorTable.REPLACE.

xccui · 2018-08-21T02:03:34Z

...braries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/BuiltInMethods.scala

@@ -118,6 +118,13 @@ object BuiltInMethods {
    Types.lookupMethod(
      classOf[ScalarFunctions], "concat_ws", classOf[String], classOf[Array[String]])

+  val REPLACE = Types.lookupMethod(


The REPLACE method has been provided in org.apache.calcite.util.BuiltInMethod. There's no need to duplicate it here.

The same reason like the above. The calcite builtin REPLACE can not handle null parameters, which may throw NPE.

xccui · 2018-08-21T02:11:14Z

...es/flink-table/src/main/scala/org/apache/flink/table/runtime/functions/ScalarFunctions.scala

+    * Returns the string str with all non-overlapping occurrences
+    * of search replaced with replacement.
+    */
+  def replace(str: String, search: String, replacement: String): String = {


The replace() method has been provided in org.apache.calcite.runtime.SqlFunctions.

The same reason like the above. The calcite builtin replace can not handle null parameters, which may throw NPE.
The builtin replace is

public static String replace(String s, String search, String replacement) { return s.replace(search, replacement); }

and in java.lang.String#replace it is

public String replace(CharSequence target, CharSequence replacement) { return Pattern.compile(target.toString(), Pattern.LITERAL).matcher( this).replaceAll(Matcher.quoteReplacement(replacement.toString())); }

Here it calls toString method, that will cause Exception when any of the thress params is null.

xccui · 2018-08-21T06:01:45Z

Hi @Guibo-Pan, in SQL convention, any null argument will lead to a null result. Thus when generating a scalar function call, all the arguments (expressions) will be first checked to ensure not null. See

flink/flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/CodeGenerator.scala

Line 406 in d5de2bc

|if (${fieldExpr.nullTerm}) {

Guibo-Pan · 2018-08-21T06:32:49Z

Thanks for your patient review, @xccui . I have update the docs and styles.

I am not so familiar with CodeGenerator so far. At the comment you said:

in SQL convention, any null argument will lead to a null result. Thus when generating a scalar function call, all the arguments (expressions) will be first checked to ensure not null

I did make some test cases on null inputs, it seems that the builtin replace can not pass. So I change the builtin replace to my replace in commit b262a4.
I think maybe something is misunderstand by me, can you give me more guidance to help me understand it? Thanks.

xccui · 2018-08-21T07:03:14Z

Hi @Guibo-Pan, see xccui@e374c3e for an example. Besides, merging commits from other branches into an active PR leads to chaos. Use git rebase instead.

Guibo-Pan · 2018-08-21T09:48:17Z

@xccui thanks very much, I've updated it, and used rebase instead. Can you take a second look on it? thanks.

xccui · 2018-09-21T03:58:50Z

Hi @Guibo-Pan, I wonder if you could rebase your code to resolve these conflicts.
Thanks, Xingcan

Change-Id: I7da6b3e81e1f9f9a177b54d6f0abb0fa41bcb7f7

Change-Id: I2b6d95d63b438e3d3e9d176ab5c18c4718fdeb1e

Change-Id: I47e5ca5b75f374d824df4f12aff61382fef7a186

Change-Id: Ibeb0e1bf2cc68bb3cfeb6ab7c1f7d791096d017b

Guibo-Pan · 2018-09-26T09:34:42Z

Hi @xccui, I've removed the conflicts. Thanks~

xccui · 2018-09-26T13:13:11Z

Thanks for the update @Guibo-Pan. I'll have a final check and merge this.
Best, Xingcan

This closes apache#6576.

This closes apache#6576. (cherry picked from commit 24af70f)

yanghua reviewed Aug 19, 2018

View reviewed changes

xccui requested changes Aug 21, 2018

View reviewed changes

Guibo-Pan force-pushed the FLINK-10145 branch from 23c01ab to 4b262a4 Compare August 21, 2018 06:47

Guibo-Pan force-pushed the FLINK-10145 branch from 4b262a4 to 095283a Compare August 21, 2018 09:19

Guibo-Pan added 4 commits September 26, 2018 11:31

[FLINK-10145][table] Add replace supported in TableAPI and SQL

66032ac

Change-Id: I7da6b3e81e1f9f9a177b54d6f0abb0fa41bcb7f7

[FLINK-10145][table] Add replace supported in TableAPI and SQL

8143883

Change-Id: I2b6d95d63b438e3d3e9d176ab5c18c4718fdeb1e

[FLINK-10145][table] Add replace supported in TableAPI and SQL

861fa80

Change-Id: I47e5ca5b75f374d824df4f12aff61382fef7a186

[FLINK-10145][table] Add replace supported in TableAPI and SQL

1f6b375

Change-Id: Ibeb0e1bf2cc68bb3cfeb6ab7c1f7d791096d017b

Guibo-Pan force-pushed the FLINK-10145 branch from 7bb1c50 to 1f6b375 Compare September 26, 2018 09:24

asfgit closed this in 24af70f Sep 27, 2018

zhangxinyu1 pushed a commit to zhangxinyu1/flink that referenced this pull request Sep 27, 2018

[FLINK-10145] [table] Add replace function in Table API and SQL

13aca23

This closes apache#6576.

Clarkkkkk pushed a commit to Clarkkkkk/flink that referenced this pull request Mar 7, 2019

[FLINK-10145] [table] Add replace function in Table API and SQL

0fe06cf

This closes apache#6576. (cherry picked from commit 24af70f)

rmetzger added the component=API/TableSQL label Mar 18, 2019

flinkbot added component=TableSQL/API and removed component=API/TableSQL labels Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-10145][table] Add replace supported in TableAPI and SQL #6576

[FLINK-10145][table] Add replace supported in TableAPI and SQL #6576

Guibo-Pan commented Aug 18, 2018

Guibo-Pan commented Aug 19, 2018

yanghua left a comment

yanghua Aug 19, 2018

yanghua Aug 19, 2018

yanghua Aug 19, 2018

yanghua Aug 19, 2018

yanghua Aug 19, 2018

yanghua Aug 19, 2018

Guibo-Pan commented Aug 19, 2018

xccui left a comment

xccui Aug 21, 2018

xccui Aug 21, 2018

xccui Aug 21, 2018

xccui Aug 21, 2018

xccui Aug 21, 2018

xccui Aug 21, 2018

xccui Aug 21, 2018

Guibo-Pan Aug 21, 2018

xccui Aug 21, 2018

Guibo-Pan Aug 21, 2018

xccui Aug 21, 2018

Guibo-Pan Aug 21, 2018

xccui commented Aug 21, 2018

Guibo-Pan commented Aug 21, 2018

xccui commented Aug 21, 2018

Guibo-Pan commented Aug 21, 2018 •

edited

xccui commented Sep 21, 2018

Guibo-Pan commented Sep 26, 2018

xccui commented Sep 26, 2018

[FLINK-10145][table] Add replace supported in TableAPI and SQL #6576

[FLINK-10145][table] Add replace supported in TableAPI and SQL #6576

Conversation

Guibo-Pan commented Aug 18, 2018

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Guibo-Pan commented Aug 19, 2018

yanghua 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

Guibo-Pan commented Aug 19, 2018

xccui 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

xccui commented Aug 21, 2018

Guibo-Pan commented Aug 21, 2018

xccui commented Aug 21, 2018

Guibo-Pan commented Aug 21, 2018 • edited

xccui commented Sep 21, 2018

Guibo-Pan commented Sep 26, 2018

xccui commented Sep 26, 2018

Guibo-Pan commented Aug 21, 2018 •

edited