You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by Xpray <gi...@git.apache.org> on 2017/06/27 15:18:12 UTC
[GitHub] flink pull request #4200: [FLINK-7014] Expose isDeterministic interface to S...
GitHub user Xpray opened a pull request:
https://github.com/apache/flink/pull/4200
[FLINK-7014] Expose isDeterministic interface to ScalarFunction and T…
…ableFunction
Thanks for contributing to Apache Flink. Before you open your pull request, please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your pull request. For more information and/or questions please refer to the [How To Contribute guide](http://flink.apache.org/how-to-contribute.html).
In addition to going through the list, please provide a meaningful description of your changes.
- [x] General
- The pull request references the related JIRA issue ("[FLINK-XXX] Jira title text")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the JIRA id)
- [ ] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [x] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis build has passed
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Xpray/flink FLINK-7014
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4200.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 #4200
----
commit d60574380414c1862022bb25b12005d907b6d931
Author: Xpray <le...@gmail.com>
Date: 2017-06-27T14:10:06Z
[FLINK-7014] Expose isDeterministic interface to ScalarFunction and TableFunction
----
---
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 #4200: [FLINK-7014] Expose isDeterministic interface to S...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4200#discussion_r124443977
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/runtime/datastream/DataStreamUserDefinedFunctionITCase.scala ---
@@ -230,6 +231,26 @@ class DataStreamUserDefinedFunctionITCase extends StreamingMultipleProgramsTestB
assertEquals(expected.sorted, StreamITCase.testResults.sorted)
}
+
+ @Test
+ def testScalarFunctionDeterministic(): Unit = {
--- End diff --
@Xpray @sunjincheng121 I mean we can remove this test, and add a test in `ExpressionReductionTest`.
---
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 #4200: [FLINK-7014] Expose isDeterministic interface to ScalarFu...
Posted by Xpray <gi...@git.apache.org>.
Github user Xpray commented on the issue:
https://github.com/apache/flink/pull/4200
I update the code, once calcite-1860 fixed, the ignored test case will pass
---
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 #4200: [FLINK-7014] Expose isDeterministic interface to S...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/4200
---
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 #4200: [FLINK-7014] Expose isDeterministic interface to S...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4200#discussion_r124439791
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/runtime/datastream/DataStreamUserDefinedFunctionITCase.scala ---
@@ -230,6 +231,26 @@ class DataStreamUserDefinedFunctionITCase extends StreamingMultipleProgramsTestB
assertEquals(expected.sorted, StreamITCase.testResults.sorted)
}
+
+ @Test
+ def testScalarFunctionDeterministic(): Unit = {
--- End diff --
Yes, agree with @wuchong , Add a test in `ExpressionReductionTest` is correct. and I think it's better to add a exception test case.
---
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 #4200: [FLINK-7014] Expose isDeterministic interface to S...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4200#discussion_r124457316
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/ExpressionReductionTest.scala ---
@@ -422,4 +423,50 @@ class ExpressionReductionTest extends TableTestBase {
util.verifyTable(result, expected)
}
+ @Test(expected = classOf[NullPointerException])
+ def testReduceDeterministicUDF(): Unit = {
--- End diff --
The exception isn't caused by Flink, to check an unexpected exception is not a good way. I suggest to mark this test as `@Ignore` and add a TODO comment on the top of the test. After CALCITE-1860 is fixed, we can reopen this test or check&remove this test.
---
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 #4200: [FLINK-7014] Expose isDeterministic interface to ScalarFu...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/4200
Thanks for updating and rebasing, the changes look good to me now.
+1 to merge
---
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 #4200: [FLINK-7014] Expose isDeterministic interface to S...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/4200#discussion_r124504426
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/ExpressionReductionTest.scala ---
@@ -458,4 +459,52 @@ class ExpressionReductionTest extends TableTestBase {
util.verifySql(sqlQuery, expected)
}
+ // todo this NPE is caused by Calcite, it shall pass when [CALCITE-1860] is fixed
+ @Ignore
+ @Test(expected = classOf[NullPointerException])
--- End diff --
What is the expected result of the test given that CALCITE-1860 is fixed?
Since the test is ignored, we should implement the test such that is validates the expected result.
---
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 #4200: [FLINK-7014] Expose isDeterministic interface to S...
Posted by Xpray <gi...@git.apache.org>.
Github user Xpray commented on a diff in the pull request:
https://github.com/apache/flink/pull/4200#discussion_r124442541
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/runtime/datastream/DataStreamUserDefinedFunctionITCase.scala ---
@@ -230,6 +231,26 @@ class DataStreamUserDefinedFunctionITCase extends StreamingMultipleProgramsTestB
assertEquals(expected.sorted, StreamITCase.testResults.sorted)
}
+
+ @Test
+ def testScalarFunctionDeterministic(): Unit = {
--- End diff --
thanks for reviewing, I'll add more test cases in `ExpressionReductionTest `
---
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 #4200: [FLINK-7014] Expose isDeterministic interface to S...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4200#discussion_r124443845
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/utils/AggSqlFunction.scala ---
@@ -57,6 +57,8 @@ class AggSqlFunction(
) {
def getFunction: AggregateFunction[_, _] = aggregateFunction
+
+ override def isDeterministic: Boolean = aggregateFunction.isDeterministic
--- End diff --
`isDeterministic` is the base method of `SqlOperator` which is the base class of `AggSqlFunction`, `ScalarSqlFunction`, `TableSqlFunction`. So I think maybe it's not necessary to add an abstract 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 pull request #4200: [FLINK-7014] Expose isDeterministic interface to S...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4200#discussion_r124436213
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/runtime/datastream/DataStreamUserDefinedFunctionITCase.scala ---
@@ -230,6 +231,26 @@ class DataStreamUserDefinedFunctionITCase extends StreamingMultipleProgramsTestB
assertEquals(expected.sorted, StreamITCase.testResults.sorted)
}
+
+ @Test
+ def testScalarFunctionDeterministic(): Unit = {
--- End diff --
I think an IT case can't cover this change. Whether the UDF is deterministic or not, the result should be same.
---
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 #4200: [FLINK-7014] Expose isDeterministic interface to S...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4200#discussion_r124457542
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/ExpressionReductionTest.scala ---
@@ -422,4 +423,50 @@ class ExpressionReductionTest extends TableTestBase {
util.verifyTable(result, expected)
}
+ @Test(expected = classOf[NullPointerException])
+ def testReduceDeterministicUDF(): Unit = {
+ val util = streamTestUtil()
+ val table = util.addTable[(Int, Long, String)]("MyTable", 'a, 'b, 'c)
+
+ // if isDeterministic = true, will cause a Calcite NPE, which will be fixed in [CALCITE-1860]
+ val result = table
+ .select('a, 'b, 'c, DeterministicNullFunc() as 'd)
+ .where("d.isNull")
+ .select('a, 'b, 'c)
+
+ val expected: String = ""
--- End diff --
We should set the expected value here to make sure the test can pass once we reopen 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 pull request #4200: [FLINK-7014] Expose isDeterministic interface to S...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on a diff in the pull request:
https://github.com/apache/flink/pull/4200#discussion_r124506864
--- Diff: flink-libraries/flink-table/src/test/scala/org/apache/flink/table/ExpressionReductionTest.scala ---
@@ -458,4 +459,52 @@ class ExpressionReductionTest extends TableTestBase {
util.verifySql(sqlQuery, expected)
}
+ // todo this NPE is caused by Calcite, it shall pass when [CALCITE-1860] is fixed
+ @Ignore
+ @Test(expected = classOf[NullPointerException])
--- End diff --
Yes, the expected test result should not throw exception.
---
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 #4200: [FLINK-7014] Expose isDeterministic interface to S...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4200#discussion_r124440259
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/utils/AggSqlFunction.scala ---
@@ -57,6 +57,8 @@ class AggSqlFunction(
) {
def getFunction: AggregateFunction[_, _] = aggregateFunction
+
+ override def isDeterministic: Boolean = aggregateFunction.isDeterministic
--- End diff --
Can we add an abstract class for `AggSqlFunction`, `ScalarSqlFunction`,and `TableSqlFunction`. What do you think? @wuchong @Xpray
---
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 #4200: [FLINK-7014] Expose isDeterministic interface to ScalarFu...
Posted by wuchong <gi...@git.apache.org>.
Github user wuchong commented on the issue:
https://github.com/apache/flink/pull/4200
Thanks. 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 pull request #4200: [FLINK-7014] Expose isDeterministic interface to S...
Posted by sunjincheng121 <gi...@git.apache.org>.
Github user sunjincheng121 commented on a diff in the pull request:
https://github.com/apache/flink/pull/4200#discussion_r124439457
--- Diff: flink-libraries/flink-table/src/main/scala/org/apache/flink/table/functions/UserDefinedFunction.scala ---
@@ -40,6 +40,12 @@ abstract class UserDefinedFunction extends Serializable {
@throws(classOf[Exception])
def close(): Unit = {}
+ /**
+ * @return true iff a call to this function is guaranteed to always return
+ * the same result given the same parameters; true is assumed by default
+ */
+ def isDeterministic: Boolean = true
--- End diff --
I suggest explain when user need to overwrite the method, what the impact if not overwrite the method.
---
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.
---