You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by godfreyhe <gi...@git.apache.org> on 2017/04/01 06:34:25 UTC
[GitHub] flink pull request #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER...
GitHub user godfreyhe opened a pull request:
https://github.com/apache/flink/pull/3660
[FLINK-6237] [table] support RAND and RAND_INTEGER on SQL
support RAND and RAND_INTEGER with and without seed on SQL.
like:
RAND([seed]),
RAND_INTEGER([seed, ] bound)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/godfreyhe/flink FLINK-6237
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/3660.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #3660
----
commit dfdc5eab23cfe549de86fca97d2d78585e35fcb0
Author: godfreyhe <go...@163.com>
Date: 2017-04-01T06:28:49Z
[FLINK-6237] [table] support RAND and RAND_INTEGER on SQL
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER on SQL
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/3660
Unfortunately, I found two issues that need to be fixed before we can merge this. According to SQL the seed and bound can also be defined by an other column. Right now `rand(f7)` leads to a compiler exception. We should check if the arguments are constant and only then move it to the reusable area.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/3660#discussion_r109424139
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/ScalarOperators.scala ---
@@ -911,6 +911,64 @@ object ScalarOperators {
}
}
+ def generateRand(
+ randField: String,
+ seedExpr: GeneratedExpression,
+ resultType: TypeInformation[_])
+ : GeneratedExpression = {
+ val resultTerm = newName("result")
+ val resultTypeTerm = primitiveTypeTermForTypeInfo(resultType)
+ val randCode = if (seedExpr != null) {
+ s"""
+ |if ($randField == null) {
+ | ${seedExpr.code}
+ | $randField = new java.util.Random(${seedExpr.resultTerm});
+ |}
+ |$resultTypeTerm $resultTerm = $randField.nextDouble();
+ """.stripMargin
+ } else {
+ s"""
+ |if ($randField == null) {
+ | $randField = new java.util.Random();
+ |}
+ |$resultTypeTerm $resultTerm = $randField.nextDouble();
+ """.stripMargin
+ }
+
+ GeneratedExpression(resultTerm, GeneratedExpression.NEVER_NULL, randCode, resultType)
+ }
+
+ def generateRandInteger(
+ randField: String,
+ seedExpr: GeneratedExpression,
+ boundExpr: GeneratedExpression,
+ resultType: TypeInformation[_])
+ : GeneratedExpression = {
+ assert(boundExpr != null)
+ val resultTerm = newName("result")
+ val resultTypeTerm = primitiveTypeTermForTypeInfo(resultType)
+ val randCode = if (seedExpr != null) {
+ s"""
+ |if ($randField == null) {
--- End diff --
If we initialize `Random` before, we do not need this condition.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/3660#discussion_r109424315
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/expressions/SqlExpressionTest.scala ---
@@ -168,6 +168,18 @@ class SqlExpressionTest extends ExpressionTestBase {
testSqlApi("ELEMENT(ARRAY['HELLO WORLD'])", "HELLO WORLD")
}
+ @Test
+ def testRand(): Unit = {
+ val random = new java.util.Random(1)
+ testSqlApi("RAND(1)", random.nextDouble().toString)
--- End diff --
Check a second time that the next random number is different.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER on SQL
Posted by godfreyhe <gi...@git.apache.org>.
Github user godfreyhe commented on the issue:
https://github.com/apache/flink/pull/3660
@twalthr do you mean that sql like ```select a, rand(a) from table``` does not work?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/3660#discussion_r109453119
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/CodeGenerator.scala ---
@@ -1106,6 +1106,31 @@ class CodeGenerator(
requireArray(array)
generateArrayElement(this, array)
+ // RAND([seed])
+ case RAND if isNumeric(resultType) && operands.size <= 1 =>
--- End diff --
We don't need this changes if the method is implemented using a `CallGenerator` in `FunctionGenerator`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER on SQL
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/3660
@godfreyhe exactly. We should check if the argument is literal or not.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/3660#discussion_r109908139
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/expressions/SqlExpressionTest.scala ---
@@ -168,6 +168,18 @@ class SqlExpressionTest extends ExpressionTestBase {
testSqlApi("ELEMENT(ARRAY['HELLO WORLD'])", "HELLO WORLD")
}
+ @Test
+ def testRand(): Unit = {
+ val random = new java.util.Random(1)
+ testSqlApi("RAND(1)", random.nextDouble().toString)
--- End diff --
I see, I think in that case it should be OK, to keep the test as it is.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/3660
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/3660#discussion_r109424063
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/ScalarOperators.scala ---
@@ -911,6 +911,64 @@ object ScalarOperators {
}
}
+ def generateRand(
+ randField: String,
+ seedExpr: GeneratedExpression,
+ resultType: TypeInformation[_])
+ : GeneratedExpression = {
+ val resultTerm = newName("result")
+ val resultTypeTerm = primitiveTypeTermForTypeInfo(resultType)
+ val randCode = if (seedExpr != null) {
+ s"""
+ |if ($randField == null) {
--- End diff --
If we initialize `Random` before, we do not need to this condition.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/3660#discussion_r109423941
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/CodeGenerator.scala ---
@@ -1494,6 +1519,22 @@ class CodeGenerator(
}
/**
+ * Adds a reusable [[java.util.Random]] to the member area of the generated [[Function]].
+ *
+ * @return member variable term
+ */
+ def addReusableRandom(): String = {
+ val fieldTerm = newName("random")
+
+ val field =
+ s"""
+ |transient java.util.Random $fieldTerm = null;
--- End diff --
`Random` implements `Serializable`. I think we initialize `Random` already here, remove `transient` and add `final`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER on SQL
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/3660
Thanks @godfreyhe. I will merge this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER on SQL
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/3660
@godfreyhe sorry for the delay. I will rebase this PR and merge it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER on SQL
Posted by godfreyhe <gi...@git.apache.org>.
Github user godfreyhe commented on the issue:
https://github.com/apache/flink/pull/3660
thanks for your advice @fhueske @twalthr. I had update the PR.
I will implement rand and rand_integer functions for the Scala and Java Table API next immediately, or should I submit a new PR ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/3660#discussion_r109424349
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/expressions/SqlExpressionTest.scala ---
@@ -168,6 +168,18 @@ class SqlExpressionTest extends ExpressionTestBase {
testSqlApi("ELEMENT(ARRAY['HELLO WORLD'])", "HELLO WORLD")
}
+ @Test
+ def testRand(): Unit = {
+ val random = new java.util.Random(1)
+ testSqlApi("RAND(1)", random.nextDouble().toString)
+ }
+
+ @Test
+ def testRandInteger(): Unit = {
+ val random = new java.util.Random(1)
+ testSqlApi("RAND_INTEGER(1, 10)", random.nextInt(10).toString)
--- End diff --
Check a second time.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/3660#discussion_r109451391
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/ScalarOperators.scala ---
@@ -911,6 +911,64 @@ object ScalarOperators {
}
}
+ def generateRand(
--- End diff --
The ScalarOperators class is intended for basic operations such as plus, minus, equals etc. Can you add the functions to `FunctionGenerator` class?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER on SQL
Posted by godfreyhe <gi...@git.apache.org>.
Github user godfreyhe commented on the issue:
https://github.com/apache/flink/pull/3660
@twalthr, I had update this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER...
Posted by godfreyhe <gi...@git.apache.org>.
Github user godfreyhe commented on a diff in the pull request:
https://github.com/apache/flink/pull/3660#discussion_r109898654
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/expressions/SqlExpressionTest.scala ---
@@ -168,6 +168,18 @@ class SqlExpressionTest extends ExpressionTestBase {
testSqlApi("ELEMENT(ARRAY['HELLO WORLD'])", "HELLO WORLD")
}
+ @Test
+ def testRand(): Unit = {
+ val random = new java.util.Random(1)
+ testSqlApi("RAND(1)", random.nextDouble().toString)
--- End diff --
This test case can only test a expression on one row. So I will add a IT case to test rand and rand_integer on multi-rows.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER...
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/3660#discussion_r109453682
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/codegen/calls/ScalarOperators.scala ---
@@ -911,6 +911,64 @@ object ScalarOperators {
}
}
+ def generateRand(
+ randField: String,
+ seedExpr: GeneratedExpression,
+ resultType: TypeInformation[_])
+ : GeneratedExpression = {
+ val resultTerm = newName("result")
+ val resultTypeTerm = primitiveTypeTermForTypeInfo(resultType)
+ val randCode = if (seedExpr != null) {
+ s"""
+ |if ($randField == null) {
+ | ${seedExpr.code}
+ | $randField = new java.util.Random(${seedExpr.resultTerm});
--- End diff --
What happens if `seedExpr.nullTerm` is true? We need a case distinction for both null check enabled and disabled.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #3660: [FLINK-6237] [table] support RAND and RAND_INTEGER on SQL
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/3660
Thanks for the update @godfreyhe!
Looks good, IMO.
I think it would be good to add the Table API functions and documentation with this PR.
Thanks, Fabian
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---