Hello, Vladimir. I’m looked through your PR and find it questionable. Every java reference variable can be null. So, is it really necessary to highlight this fact in the code one more time?
Do we have some code style convention that requires it? Java is known as a very verbose programming language, so making it even more verbose with an annotation that doesn’t add any value to the code is a bad thing. > * I've provided multiple samples of the bugs identified by the checker. The > are SQL-level bugs as well. Can you, please, show the list of the bugs founded while adding @nullable all over the code? > 18 нояб. 2020 г., в 02:25, Vladimir Sitnikov <[email protected]> > написал(а): > > Hi, > > I've completed @nullable annotations for linq4j and core modules. > The PR is https://github.com/apache/calcite/pull/2268 > > All existing tests pass. > There might be minor tweaks, however, I expect the change is mergeable. > > if no-one objects within a week (~1 December), I'll assume lazy consensus > and commit it (see > https://www.apache.org/foundation/voting.html#LazyConsensus ) > > However, do not even think that I use lazy consensus as an escape hatch to > commit something doubtful. > On contrary, I truly believe that merging nullness annotations is the right > thing to do. > It fixes certain types of bugs, and it prevents new NPE. > The change makes it easier to use Calcite API, and it opens a way to using > https://github.com/jspecify/jspecify when it is ready. > > > Just to recap: > * The first discussions regarding the checker framework was in April > * I've provided multiple samples of the bugs identified by the checker. The > are SQL-level bugs as well. > * I completed linq4j annotation by 30 August > * Other projects use the checker framework (e.g. Guava) > * Kenneth Knowles shared a strongly positive experience with adding > nullness annotations to Beam > * Michael Mior was positive regarding the change > * I did my best to minimize the diff, so I did my best to keep the old > behavior when possible > > I assume there was more than enough time to review the approach and play > with the technology (more than 2 months), > that is why I set lazy consensus timeframe to 1 week. > > The PR touches a lot of lines, so I do not expect somebody would fully > review the PR. It would be nice to have feedback though. > The most part of the PR is the addition of the relevant @Nullable > annotations. > Of course, sometimes it triggers formatting (due to 100 char line limit) > > As I added nullness verification, I routinely rebased over the new changes > coming to the master branch. > The rebase is not hard to do. Of course, there might be conflicts, however, > the resolution was trivial. > In my experience, conflict resolution is way easier to do when nullability > annotations are added as a single commit. > > Vladimir
