[FLINK-11296][Table API & SQL] Support truncate in TableAPI #7450

XuQianJin-Stars · 2019-01-10T10:19:14Z

What is the purpose of the change

This PR add truncate as core scalar operators in then codegen.

Brief change log

(for example:)

docs/dev/table/functions.md
table/api/scala/expressionDsl.scala
table/codegen/calls/BuiltInMethods.scala
table/codegen/calls/FunctionGenerator.scala
table/expressions/mathExpressions.scala
table/validate/FunctionCatalog.scala
table/expressions/ScalarFunctionsTest.scala
table/expressions/SqlExpressionTest.scala

Verifying this change

(Please pick either of the following options)

This change added tests in ScalarOperatorsTest and SqlExpressionTest

(example:)

Added integration tests for end-to-end deployment with large payloads (100MB)
Extended integration test for recovery after master (JobManager) failure
Added test that validates that TaskInfo is transferred only once across recoveries
Manually verified the change by running a 4 node cluser with 2 JobManagers and 4 TaskManagers, a stateful streaming program, and killing one JobManager and two TaskManagers during the execution, verifying that recovery happens correctly.

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

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

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

wuchong

Thank @XuQianJin-Stars for the work. I left some comments below.

Could you add some more validation tests to ScalarFunctionsValidationTest? e.g. the second parameter is not an int, the first parameter is a String.
I think we should also support single parameter (second parameter deafult as 0)
We should also support FLOAT, INT as the first parameter as they belong to the NUMERIC family. (see org.apache.calcite.sql.type.SqlTypeFamily.NUMERIC)

wuchong · 2019-01-11T08:41:00Z

docs/dev/table/functions.md

+      </td>
+      <td>
+        <p>Returns a <i>numeric</i> after the truncation.</p>
+        <p>E.g. truncate(cast(42.345 as decimal(2, 3)), 2) to 42.34.</p>


Can we use truncate(42.345, 2) as a simpler example here?

wuchong · 2019-01-11T08:41:54Z

docs/dev/table/functions.md

+    <tr>
+      <td>
+        {% highlight text %}
+TRUNCATE(numeric1, numeric2)


Should the second parameter be an INT?

wuchong · 2019-01-11T08:42:01Z

docs/dev/table/functions.md

+       <tr>
+         <td>
+           {% highlight text %}
+numeric1.shiftLeft(numeric2)


NUMERIC1.truncate(INT) ?

wuchong · 2019-01-11T08:42:26Z

docs/dev/table/functions.md

+         </td>
+         <td>
+           <p>Returns a <i>numeric</i> after the truncation.</p>
+           <p>E.g. truncate(cast(42.345 as decimal(2, 3)), 2) to 42.34.</p>


Modify this to TalbeApi example.

wuchong · 2019-01-11T08:43:51Z

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

+    * n can be negative to cause n digits left of the decimal point of the value to become zero.
+    *
+    * @param n
+    * @return


remove the @param and @return

wuchong

Hi @XuQianJin-Stars , it looks good to me overall, I only left some minor comments below.

BTW, please do not squash commits and force-push (otherwise rebase master to solve the conflicts), this will lose the review comment location and make the reviewer have to review the PR totally again.

Please comment in the PR when you addressed all the comment.

wuchong · 2019-01-14T06:42:38Z

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

+      'f29.truncate('f30),
+      "f29.truncate(f30)",
+      "truncate(f29, f30)",
+      0.4.toString)


why not use "0.4" here ?

wuchong · 2019-01-14T06:45:14Z

...test/scala/org/apache/flink/table/expressions/validation/ScalarFunctionsValidationTest.scala

@@ -64,6 +65,42 @@ class ScalarFunctionsValidationTest extends ScalarTypesTestBase {
    testSqlApi("BIN(f16)", "101010") // Date type
  }

+  @Test(expected = classOf[ValidationException])
+  def testInvalidTruncate1(): Unit = {
+    // The all arguments is of type String


All arguments are string type

wuchong · 2019-01-14T06:58:12Z

docs/dev/table/functions.md

+{% endhighlight %}
+      </td>
+      <td>
+        <p>Returns a <i>numeric</i> after the truncation.</p>


TRUNCATE(numeric, integer) and TRUNCATE(numeric) should be placed in a row together because they are the same function.

Please explain more about description of the function. For example:

Returns numeric truncated to integer decimal places. If integer is omitted, then numeric is truncated to 0 places. integer can be negative to truncate (make zero) integer digits left of the decimal point. E.g. ...... 

XuQianJin-Stars · 2019-01-15T05:46:11Z

hi @wuchong I have modified this PR. Do you have time to review it.
best,
qianjin

wuchong · 2019-01-17T08:46:42Z

docs/dev/table/functions.md

    <tr>
      <td>
        {% highlight text %}
 TRUNCATE(numeric1, integer2)
 {% endhighlight %}
      </td>
      <td>
-        <p>Returns a <i>numeric</i> after the truncation.</p>
-        <p>E.g. truncate(42.345, 2) to 42.34.</p>
+           <p>Returns a <i>numeric</i> of truncated to <i>integer2</i> decimal places. Returns NULL if <i>numeric1</i> or <i>integer2</i> is NULL.If <i>integer2</i> is 0,the result has no decimal point or fractional part.<i>integer2</i> can be negative to cause <i>integer2</i> digits left of the decimal point of the value to become zero.This function can also pass in only one <i>numeric1</i> parameter and not set <i>Integer2</i> to use.</p>


Please describe what's the behavior when pass only one argument.

XuQianJin-Stars · 2019-01-17T09:37:43Z

hi @wuchong Ok, let me revise the description 。
best
qianjin

XuQianJin-Stars · 2019-01-17T12:22:19Z

hi @wuchong I have modified this PR. Do you have time to review it.
best,
qianjin

wuchong · 2019-01-30T04:21:04Z

Thank @XuQianJin-Stars and sorry for the late reply. The PR looks good to me now.

Will merge this soon.

XuQianJin-Stars · 2019-01-30T04:41:23Z

hi @wuchong Thank you very much. I know you are very busy.
best
qianjin

wuchong reviewed Jan 11, 2019

View reviewed changes

[FLINK-11296][Table API & SQL] Support truncate in TableAPI

28ec5f9

wuchong reviewed Jan 14, 2019

View reviewed changes

XuQianJin-Stars added 3 commits January 14, 2019 15:32

[FLINK-11296][Table API & SQL] Support truncate in TableAPI #2

8ce4421

[FLINK-11296][Table API & SQL] Support truncate in TableAPI #3

e039a9f

[FLINK-11296][Table API & SQL] Support truncate in TableAPI #4

af2c37a

wuchong reviewed Jan 17, 2019

View reviewed changes

[FLINK-11296][Table API & SQL] Support truncate in TableAPI #4

ab6dbd5

asfgit closed this in e154375 Jan 30, 2019

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-11296][Table API & SQL] Support truncate in TableAPI #7450

[FLINK-11296][Table API & SQL] Support truncate in TableAPI #7450

XuQianJin-Stars commented Jan 10, 2019

wuchong left a comment

wuchong Jan 11, 2019

wuchong Jan 11, 2019

wuchong Jan 11, 2019

wuchong Jan 11, 2019

wuchong Jan 11, 2019

wuchong left a comment

wuchong Jan 14, 2019

wuchong Jan 14, 2019

wuchong Jan 14, 2019

XuQianJin-Stars commented Jan 15, 2019

wuchong Jan 17, 2019

XuQianJin-Stars commented Jan 17, 2019

XuQianJin-Stars commented Jan 17, 2019

wuchong commented Jan 30, 2019

XuQianJin-Stars commented Jan 30, 2019

[FLINK-11296][Table API & SQL] Support truncate in TableAPI #7450

[FLINK-11296][Table API & SQL] Support truncate in TableAPI #7450

Conversation

XuQianJin-Stars commented Jan 10, 2019

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

wuchong 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

wuchong 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

XuQianJin-Stars commented Jan 15, 2019

Choose a reason for hiding this comment

XuQianJin-Stars commented Jan 17, 2019

XuQianJin-Stars commented Jan 17, 2019

wuchong commented Jan 30, 2019

XuQianJin-Stars commented Jan 30, 2019