You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by apivovarov <gi...@git.apache.org> on 2016/09/10 05:25:40 UTC
[GitHub] flink pull request #2490: [FLINK-4609] Remove redundant check for null in Cr...
GitHub user apivovarov opened a pull request:
https://github.com/apache/flink/pull/2490
[FLINK-4609] Remove redundant check for null in CrossOperator
https://issues.apache.org/jira/browse/FLINK-4609
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apivovarov/flink FLINK-4609
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2490.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 #2490
----
commit 272007fc8f350b1b998f28762aade6760b588c73
Author: Alexander Pivovarov <ap...@gmail.com>
Date: 2016-09-10T05:24:43Z
[FLINK-4609] Remove redundant check for null in CrossOperator
----
---
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 #2490: [FLINK-4609] Remove redundant check for null in Cr...
Posted by apivovarov <gi...@git.apache.org>.
Github user apivovarov commented on a diff in the pull request:
https://github.com/apache/flink/pull/2490#discussion_r78283317
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/operators/CrossOperator.java ---
@@ -133,10 +133,6 @@ public DefaultCross(DataSet<I1> input1, DataSet<I2> input2, CrossHint hint, Stri
new TupleTypeInfo<Tuple2<I1, I2>>(input1.getType(), input2.getType()),
hint, defaultName);
- if (input1 == null || input2 == null) {
- throw new NullPointerException();
- }
-
--- End diff --
if input1 or/and input2 are null CrossOperator will throw NPE on line 133
I also added check for null to TwoInputOperator
---
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 #2490: [FLINK-4609] Remove redundant check for null in Cr...
Posted by apivovarov <gi...@git.apache.org>.
Github user apivovarov commented on a diff in the pull request:
https://github.com/apache/flink/pull/2490#discussion_r78287335
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/operators/CrossOperator.java ---
@@ -129,14 +129,11 @@ private String getDefaultName() {
public DefaultCross(DataSet<I1> input1, DataSet<I2> input2, CrossHint hint, String defaultName) {
- super(input1, input2, new DefaultCrossFunction<I1, I2>(),
+ super(Preconditions.checkNotNull(input1, "input1 is null"),
--- End diff --
DefaultCross calls `input1.getType()` and `input2.getType()` before calling super() on line 134. So, if we add null check to super class (e.g. TwoInputOperator) it will not work for DefaultCross
---
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 #2490: [FLINK-4609] Remove redundant check for null in Cr...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2490#discussion_r78273103
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/operators/CrossOperator.java ---
@@ -133,10 +133,6 @@ public DefaultCross(DataSet<I1> input1, DataSet<I2> input2, CrossHint hint, Stri
new TupleTypeInfo<Tuple2<I1, I2>>(input1.getType(), input2.getType()),
hint, defaultName);
- if (input1 == null || input2 == null) {
- throw new NullPointerException();
- }
-
--- End diff --
we could instead move the null check into the `super()` call using `Preconditions.checkNotNull()`
---
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 #2490: [FLINK-4609] Remove redundant check for null in CrossOper...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2490
All right, +1 then
---
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 #2490: [FLINK-4609] Remove redundant check for null in CrossOper...
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/2490
Merging ...
---
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 #2490: [FLINK-4609] Remove redundant check for null in Cr...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2490
---
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 #2490: [FLINK-4609] Remove redundant check for null in Cr...
Posted by apivovarov <gi...@git.apache.org>.
Github user apivovarov commented on a diff in the pull request:
https://github.com/apache/flink/pull/2490#discussion_r78631478
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/operators/CrossOperator.java ---
@@ -129,14 +129,11 @@ private String getDefaultName() {
public DefaultCross(DataSet<I1> input1, DataSet<I2> input2, CrossHint hint, String defaultName) {
- super(input1, input2, new DefaultCrossFunction<I1, I2>(),
+ super(Preconditions.checkNotNull(input1, "input1 is null"),
--- End diff --
@greghogan
---
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 #2490: [FLINK-4609] Remove redundant check for null in Cr...
Posted by apivovarov <gi...@git.apache.org>.
Github user apivovarov commented on a diff in the pull request:
https://github.com/apache/flink/pull/2490#discussion_r78285322
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/operators/CrossOperator.java ---
@@ -133,10 +133,6 @@ public DefaultCross(DataSet<I1> input1, DataSet<I2> input2, CrossHint hint, Stri
new TupleTypeInfo<Tuple2<I1, I2>>(input1.getType(), input2.getType()),
hint, defaultName);
- if (input1 == null || input2 == null) {
- throw new NullPointerException();
- }
-
--- End diff --
Ok, I added null check for input1 and input2 with a message inline with calling super in DefaultCross
---
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 #2490: [FLINK-4609] Remove redundant check for null in CrossOper...
Posted by apivovarov <gi...@git.apache.org>.
Github user apivovarov commented on the issue:
https://github.com/apache/flink/pull/2490
Ok, I added null check for input1 and input2 with a message before calling super in DefaultCross
---
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 #2490: [FLINK-4609] Remove redundant check for null in CrossOper...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/2490
Thanks for the contribution @apivovarov. The PR looks good except for the modified test which fails.
---
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 #2490: [FLINK-4609] Remove redundant check for null in Cr...
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/2490#discussion_r78283727
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/operators/CrossOperator.java ---
@@ -133,10 +133,6 @@ public DefaultCross(DataSet<I1> input1, DataSet<I2> input2, CrossHint hint, Stri
new TupleTypeInfo<Tuple2<I1, I2>>(input1.getType(), input2.getType()),
hint, defaultName);
- if (input1 == null || input2 == null) {
- throw new NullPointerException();
- }
-
--- End diff --
yes, but if you used the preconditions check you could supply a useful error message, for example stating which input was actually null.
---
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 #2490: [FLINK-4609] Remove redundant check for null in CrossOper...
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/2490
This isn't improving performance but moving the null checks before first access and removing the duplicate `DataSet` references.
---
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 #2490: [FLINK-4609] Remove redundant check for null in Cr...
Posted by apivovarov <gi...@git.apache.org>.
Github user apivovarov commented on a diff in the pull request:
https://github.com/apache/flink/pull/2490#discussion_r78288200
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/operators/CrossOperator.java ---
@@ -129,14 +129,11 @@ private String getDefaultName() {
public DefaultCross(DataSet<I1> input1, DataSet<I2> input2, CrossHint hint, String defaultName) {
- super(input1, input2, new DefaultCrossFunction<I1, I2>(),
+ super(Preconditions.checkNotNull(input1, "input1 is null"),
--- End diff --
I also added null check to TwoInputOperator
And removed `input1` and `input2` fields from DefaultCross because TwoInputOperator already has them
---
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 #2490: [FLINK-4609] Remove redundant check for null in Cr...
Posted by apivovarov <gi...@git.apache.org>.
Github user apivovarov commented on a diff in the pull request:
https://github.com/apache/flink/pull/2490#discussion_r79441284
--- Diff: flink-java/src/test/java/org/apache/flink/api/java/sca/UdfAnalyzerTest.java ---
@@ -971,10 +971,7 @@ public void testForwardWithAtLeastOneIterationAssumptionForJavac() {
public void reduce(Iterable<Tuple2<Long, Long>> values, Collector<Long> out) throws Exception {
Long id = 0L;
for (Tuple2<Long, Long> value : values) {
- id = value.f0;
--- End diff --
Just reverted this change. Thank you!
---
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 #2490: [FLINK-4609] Remove redundant check for null in Cr...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/2490#discussion_r79404090
--- Diff: flink-java/src/test/java/org/apache/flink/api/java/sca/UdfAnalyzerTest.java ---
@@ -971,10 +971,7 @@ public void testForwardWithAtLeastOneIterationAssumptionForJavac() {
public void reduce(Iterable<Tuple2<Long, Long>> values, Collector<Long> out) throws Exception {
Long id = 0L;
for (Tuple2<Long, Long> value : values) {
- id = value.f0;
--- End diff --
This change causes a test to fail and should be reverted.
The UdfAnalyzer is a component that inspects the bytecode of user-defined functions to infer how they access input values. By removing the if condition (which is not evaluated by the analyzer), the analyzer can better infer the behavior of the function produces a different output.
---
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 #2490: [FLINK-4609] Remove redundant check for null in Cr...
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on a diff in the pull request:
https://github.com/apache/flink/pull/2490#discussion_r78286377
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/operators/CrossOperator.java ---
@@ -129,14 +129,11 @@ private String getDefaultName() {
public DefaultCross(DataSet<I1> input1, DataSet<I2> input2, CrossHint hint, String defaultName) {
- super(input1, input2, new DefaultCrossFunction<I1, I2>(),
+ super(Preconditions.checkNotNull(input1, "input1 is null"),
--- End diff --
Can we do the preconditions check in `TwoInputOperator` rather than the subclasses?
---
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 #2490: [FLINK-4609] Remove redundant check for null in CrossOper...
Posted by StephanEwen <gi...@git.apache.org>.
Github user StephanEwen commented on the issue:
https://github.com/apache/flink/pull/2490
Playing Devil's advocate here:
Does this really improve anything meaningful? This is an API-level operator, not anything runtime related, so the additional null check is not really important. The code is not getting simpler in my opinion as well. It worked well before, and every change may introduce another bug.
Why not focus energies on improvements where the system really gains?
---
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 #2490: [FLINK-4609] Remove redundant check for null in CrossOper...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the issue:
https://github.com/apache/flink/pull/2490
Thanks for the fast update @apivovarov.
PR is good 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.
---