You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/07/24 06:33:04 UTC

[GitHub] [beam] jayendra13 opened a new pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

jayendra13 opened a new pull request #12366:
URL: https://github.com/apache/beam/pull/12366


   …sql:datacatalog
   
   Fixing the nullability issues for sub-module sdks:java:extensions:sql:datacatalog
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [ ] Format the pull request title like `[BEAM-XXX] Fixes bug in ApproximateQuantiles`, where you replace `BEAM-XXX` with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
    - [ ] Update `CHANGES.md` with noteworthy changes.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
   
   Post-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
   --- | --- | --- | --- | --- | --- | ---
   Go | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
   Java | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/badge/i
 con)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[![Build Status](htt
 ps://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [![Build Status](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/badge/icon)](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
   Python | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_
 Py_VR_Dataflow/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_P
 ostCommit_Python_VR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
   XLang | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
   
   Pre-Commit Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   
   --- |Java | Python | Go | Website
   --- | --- | --- | --- | ---
   Non-portable | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [![Build Status](https://ci-beam.apache.org/job/b
 eam_PreCommit_Website_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
   Portable | --- | [![Build Status](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/badge/icon)](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
   
   See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] jayendra13 commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
jayendra13 commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-663507303


   Run SQL_Java11 PreCommit


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] kennknowles commented on a change in pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #12366:
URL: https://github.com/apache/beam/pull/12366#discussion_r461201686



##########
File path: sdks/java/extensions/sql/datacatalog/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/datacatalog/DataCatalogBigQueryIT.java
##########
@@ -66,12 +66,15 @@
           });
     }
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(0)
     public String dialectName;
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(1)
     public Class<? extends QueryPlanner> queryPlanner;
 
+    @SuppressWarnings("nullness")

Review comment:
       Yes, my mistake. I used the name `reference` when I needed to use the positional parameter `#1`. It should be fixed now.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] jayendra13 commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:sql:datacatalog

Posted by GitBox <gi...@apache.org>.
jayendra13 commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-671006427


   Can some one point me how to disable that. I know we have to modify some groovy script under `.test-infra/jenkins`, but don't know exactly where and how ?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] jayendra13 commented on a change in pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
jayendra13 commented on a change in pull request #12366:
URL: https://github.com/apache/beam/pull/12366#discussion_r460366521



##########
File path: sdks/java/extensions/sql/datacatalog/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/datacatalog/DataCatalogBigQueryIT.java
##########
@@ -66,12 +66,15 @@
           });
     }
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(0)
     public String dialectName;
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(1)
     public Class<? extends QueryPlanner> queryPlanner;
 
+    @SuppressWarnings("nullness")

Review comment:
       Using `checkArgumentNotNull `
   ```
         readPipeline
             .getOptions()
             .as(BeamSqlPipelineOptions.class)
             .setPlannerName(checkArgumentNotNull(queryPlanner.getCanonicalName()));
   ```
   throws 
   ```
   /home/jayendra/beam/sdks/java/extensions/sql/datacatalog/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/datacatalog/DataCatalogBigQueryIT.java:101: error: [flowexpr.parse.error.postcondition] error parsing the postcondition expression for checkArgumentNotNull(T)
             .setPlannerName(checkArgumentNotNull(queryPlanner.getCanonicalName()));
                                                 ^
     cannot parse the expression Invalid 'reference' because identifier not found
   ```
   Which seems to be related to https://github.com/typetools/checker-framework/issues/752 .
   I checked the other places in the code where `checkArgumentNotNull` has been used, all the uses are under the `runner.dataflow` package where null-cheking has not been applied yet.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] kennknowles commented on a change in pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #12366:
URL: https://github.com/apache/beam/pull/12366#discussion_r460357318



##########
File path: sdks/java/extensions/sql/datacatalog/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/datacatalog/DataCatalogBigQueryIT.java
##########
@@ -66,12 +66,15 @@
           });
     }
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(0)
     public String dialectName;
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(1)
     public Class<? extends QueryPlanner> queryPlanner;
 
+    @SuppressWarnings("nullness")

Review comment:
       Maybe... in this case I realize that the string is probably used to create a planner by reflection. I don't know if that will work. If it does, then that is great. If it does not work, then you'll want to check for null and throw an exception. I've created a new Beam module to make this easy: `org.apache.beam.sdk.utils.Preconditions.checkArgumentNotNull` returns a non-nullable value after checking.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] jayendra13 commented on a change in pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
jayendra13 commented on a change in pull request #12366:
URL: https://github.com/apache/beam/pull/12366#discussion_r461343509



##########
File path: sdks/java/extensions/sql/datacatalog/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/datacatalog/DataCatalogBigQueryIT.java
##########
@@ -66,12 +66,15 @@
           });
     }
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(0)
     public String dialectName;
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(1)
     public Class<? extends QueryPlanner> queryPlanner;
 
+    @SuppressWarnings("nullness")

Review comment:
       So should I put back `@SuppressWarnings("nullness")` for the method, but that would be method level suppression. What better we can do here ?

##########
File path: sdks/java/extensions/sql/datacatalog/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/datacatalog/DataCatalogBigQueryIT.java
##########
@@ -66,12 +66,15 @@
           });
     }
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(0)
     public String dialectName;
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(1)
     public Class<? extends QueryPlanner> queryPlanner;
 
+    @SuppressWarnings("nullness")

Review comment:
       So should I put back `@SuppressWarnings("nullness")` for the method, but that would be method level suppression. May be we can pull out `runPipeline` part to separate method and apply `SuppressWarnings` to only that pulled out method. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] kennknowles commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:sql:datacatalog

Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-671362622


   Here: https://github.com/apache/beam/blob/de8ff705145cbbc41bea7750a0a5d3553924ab3a/buildSrc/src/main/groovy/org/apache/beam/gradle/BeamModulePlugin.groovy#L763
   
   This block should be skipped if `compileAndRunTestsWithJava11` is set and Gradle's configuration phase runs using JDK8. I think it would be fine to just skip that block whenever `compileAndRunTestsWithJava11` is set.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] jayendra13 commented on a change in pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
jayendra13 commented on a change in pull request #12366:
URL: https://github.com/apache/beam/pull/12366#discussion_r460356514



##########
File path: sdks/java/extensions/sql/datacatalog/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/datacatalog/DataCatalogBigQueryIT.java
##########
@@ -66,12 +66,15 @@
           });
     }
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(0)
     public String dialectName;
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(1)
     public Class<? extends QueryPlanner> queryPlanner;
 
+    @SuppressWarnings("nullness")

Review comment:
       gotcha so I am doing `queryPlanner.getCanonicalName()` --> `queryPlanner.getName()`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] jayendra13 removed a comment on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
jayendra13 removed a comment on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-663507303


   Run SQL_Java11 PreCommit


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] tysonjh commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:sql:datacatalog

Posted by GitBox <gi...@apache.org>.
tysonjh commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-665175699


   > Ah, yes. To run the same commands as the Java11 builds you must set some properties. For example on mac:
   > 
   > ```
   > git checkout github/pr/12366 # I have fetch spec set up like this
   > 
   > ./gradlew \
   >     -PcompileAndRunTestsWithJava11 \
   >     -Pjava11home=/Library/Java/JavaVirtualMachines/jdk-11-latest/Contents/Home \
   >     :sdks:java:extensions:sql:datacatalog:compileTestJava
   > ```
   > 
   > I actually got a different failure: https://gradle.com/s/gegzdydsbu2si
   > 
   > ```
   > Execution failed for task ':sdks:java:extensions:sql:datacatalog:compileTestJava'.
   > > release version 11 not supported
   > ```
   > 
   > That seems a bit odd. I think I did not reproduce the problem properly or there are differences between my JDK11 and the one on Jenkins.
   > 
   > CC @tysonjh as we talked the other day about whether it mattered to make these unique Gradle targets versus just configuring these properties in the Jenkins job.
   
   When I run with Java 11 I use the following (example from different task):
   
   ./gradlew clean
   ./gradlew -Dorg.gradle.java.home=/usr/local/buildtools/java/jdk11 :runners:direct-java:validatesRunner --scan


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] kennknowles merged pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:sql:datacatalog

Posted by GitBox <gi...@apache.org>.
kennknowles merged pull request #12366:
URL: https://github.com/apache/beam/pull/12366


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] kennknowles commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:sql:datacatalog

Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-671363224


   It also looks like `-PskipCheckerFramework` is supported by kelloggm/checkerframework-gradle-plugin so you can just add that flag to the Jenkins job. That is probably best.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] kennknowles commented on a change in pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #12366:
URL: https://github.com/apache/beam/pull/12366#discussion_r460355720



##########
File path: sdks/java/extensions/sql/datacatalog/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/datacatalog/DataCatalogBigQueryIT.java
##########
@@ -66,12 +66,15 @@
           });
     }
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(0)
     public String dialectName;
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(1)
     public Class<? extends QueryPlanner> queryPlanner;
 
+    @SuppressWarnings("nullness")

Review comment:
       The checkerframework nullness analysis is not a heuristic warning framework like spotbugs/findbugs. It is a new and improved type system, so the idea is to always satisfy it by the right types everywhere.
   
   In this case, the [javadoc for Class#getCanonicalName](https://github.com/typetools/jdk/blob/af987068c65767e101a3c58919cbae15ad08218b/src/java.base/share/classes/java/lang/Class.java#L1648) says "the canonical name of the class if it exists, otherwise null". So the checkerframework team has put a `@Nullable` annotation on the method. I actually did not realize this method was nullable. That is pretty annoying. So we might want to switch to just `getName` sometimes or otherwise use `MoreObjects.firstNonNull` to give a default string.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] kennknowles commented on a change in pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
kennknowles commented on a change in pull request #12366:
URL: https://github.com/apache/beam/pull/12366#discussion_r460258240



##########
File path: sdks/java/extensions/sql/datacatalog/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/datacatalog/DataCatalogBigQueryIT.java
##########
@@ -66,12 +66,15 @@
           });
     }
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(0)
     public String dialectName;
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(1)
     public Class<? extends QueryPlanner> queryPlanner;
 
+    @SuppressWarnings("nullness")

Review comment:
       I would prefer not to suppress the warnings, but to fix them. Or if they are suppressed, we need a comment explaining why. I know this is just test code, but good test code matters too!




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] jayendra13 commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:sql:datacatalog

Posted by GitBox <gi...@apache.org>.
jayendra13 commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-671367166


   Run SQL_Java11 PreCommit


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] jayendra13 commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
jayendra13 commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-663798158


   > Hi Jayendra, thanks for volunteering to fix these issues.
   > 
   > The SQL_Java11 precommit failure doesn't look like a flake, it seems like this change introduced an error. Please take a look.
   > 
   > ```
   > 05:09:22 > Task :sdks:java:extensions:sql:datacatalog:compileTestJava FAILED
   > 05:09:22 error: option -Xbootclasspath/p: cannot be used together with --release
   > 05:09:22 Usage: javac <options> <source files>
   > 05:09:22 use --help for a list of possible options
   > ```
   
   
   When I run that task locally I don't get any error.
   
   ```
   jayendra@alienware:~/beam$   ./gradlew :sdks:java:extensions:sql:datacatalog:compileTestJava
   To honour the JVM settings for this build a new JVM will be forked. Please consider using the daemon: https://docs.gradle.org/5.2.1/userguide/gradle_daemon.html.
   Daemon will be stopped at the end of the build stopping after processing
   Configuration on demand is an incubating feature.
   
   Deprecated Gradle features were used in this build, making it incompatible with Gradle 6.0.
   Use '--warning-mode all' to show the individual deprecation warnings.
   See https://docs.gradle.org/5.2.1/userguide/command_line_interface.html#sec:command_line_warnings
   
   BUILD SUCCESSFUL in 42s
   73 actionable tasks: 37 executed, 1 from cache, 35 up-to-date
   ```
   is it something specific to java version ?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] jayendra13 commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:sql:datacatalog

Posted by GitBox <gi...@apache.org>.
jayendra13 commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-667464736


   > Filed [kelloggm/checkerframework-gradle-plugin#117](https://github.com/kelloggm/checkerframework-gradle-plugin/issues/117). We don't have to wait for a fix, though. We can just disable checker for Java 11 for now, and we will probably at some point switch to Tyson's invocation for most tests - compiling and running on JRE11 but with Java 8 settings.
   
   Disable checker(for java 11) for whole project or for just this package ?


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] jayendra13 commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:sql:datacatalog

Posted by GitBox <gi...@apache.org>.
jayendra13 commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-671378392


   > It also looks like `-PskipCheckerFramework` is supported by kelloggm/checkerframework-gradle-plugin so you can just add that flag to the Jenkins job. That is probably best.
   
   It's was there so just triggered the job.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] ibzib commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
ibzib commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-663650059


   Hi Jayendra, thanks for volunteering to fix these issues.
   
   The SQL_Java11 precommit failure doesn't look like a flake, it seems like this change introduced an error. Please take a look.
   
   ```
   05:09:22 > Task :sdks:java:extensions:sql:datacatalog:compileTestJava FAILED
   05:09:22 error: option -Xbootclasspath/p: cannot be used together with --release
   05:09:22 Usage: javac <options> <source files>
   05:09:22 use --help for a list of possible options
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] tysonjh commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:sql:datacatalog

Posted by GitBox <gi...@apache.org>.
tysonjh commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-670165578


   > > Filed [kelloggm/checkerframework-gradle-plugin#117](https://github.com/kelloggm/checkerframework-gradle-plugin/issues/117). We don't have to wait for a fix, though. We can just disable checker for Java 11 for now, and we will probably at some point switch to Tyson's invocation for most tests - compiling and running on JRE11 but with Java 8 settings.
   > 
   > Disable checker(for java 11) for whole project or for just this package ?
   
   Disable the checker for the whole project.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] kennknowles commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
kennknowles commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-665139853






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] aromanenko-dev commented on pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
aromanenko-dev commented on pull request #12366:
URL: https://github.com/apache/beam/pull/12366#issuecomment-663432922


   Run SQL_Java11 PreCommit


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] jayendra13 commented on a change in pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:…

Posted by GitBox <gi...@apache.org>.
jayendra13 commented on a change in pull request #12366:
URL: https://github.com/apache/beam/pull/12366#discussion_r460353530



##########
File path: sdks/java/extensions/sql/datacatalog/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/datacatalog/DataCatalogBigQueryIT.java
##########
@@ -66,12 +66,15 @@
           });
     }
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(0)
     public String dialectName;
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(1)
     public Class<? extends QueryPlanner> queryPlanner;
 
+    @SuppressWarnings("nullness")

Review comment:
       I got this after enabling the checkerframework
   ```
   /Users/jayendrap/lab/beam/sdks/java/extensions/sql/datacatalog/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/datacatalog/DataCatalogBigQueryIT.java:100: error: [argument.type.incompatible] incompatible types in argument.
             .setPlannerName(queryPlanner.getCanonicalName());
                                                          ^
     found   : @Initialized @Nullable String
     required: @Initialized @NonNull String
   ```
   I am assuming that it is thrown as `queryPlanner` is uninitialized so compiler is inferring that `queryPlanner.getCanonicalName()` would give `null` and hence I suppressed that. Am I correct here, or I can't understand any other reason for this nullness. 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [beam] jayendra13 commented on a change in pull request #12366: [BEAM-10572] Eliminate nullability errors from :sdks:java:extensions:sql:datacatalog

Posted by GitBox <gi...@apache.org>.
jayendra13 commented on a change in pull request #12366:
URL: https://github.com/apache/beam/pull/12366#discussion_r467930130



##########
File path: sdks/java/extensions/sql/datacatalog/src/test/java/org/apache/beam/sdk/extensions/sql/meta/provider/datacatalog/DataCatalogBigQueryIT.java
##########
@@ -66,12 +66,15 @@
           });
     }
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(0)
     public String dialectName;
 
+    @SuppressWarnings("initialization.fields.uninitialized")
     @Parameterized.Parameter(1)
     public Class<? extends QueryPlanner> queryPlanner;
 
+    @SuppressWarnings("nullness")

Review comment:
       I think I should mark this as `Resolved`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org