Hi Vladimir, Thanks for your effort. I still believe in the value of nullable annotations.
Please see my comments below > I'm puzzled. First you say "annotations are beneficial for code > readability", then > you proceed with "annotations does not make the code more readable". For ordinary Java code, variables are nullable by default, and only a few variables are non-nullable (I think). But Calcite is different, as most variables are non-nullable (as you have indicated). If so, it would be reasonable to annotate nullable variables only. However, I think it would be nice if we could document it somewhere, so other developers will follow the convention without being confused. > It won't be difficult to merge. Git works just fine. I mean that a large PR requires a long time to review. During this time, you have to resolve conflicts many times, which is a large amount of effort. This is not a problem, of course, if you do not mind it. Best, Liya Fan On Fri, Nov 20, 2020 at 7:35 AM Vladimir Sitnikov < [email protected]> wrote: > Liya Fan>is it possible to break the PR into a number of small ones? > > I can split the commit in two for review-only purposes: a commit that > refactors the code, and the second commit that adds @Nullable annotations. > Here you go: https://github.com/apache/calcite/pull/2273 (see > [CALCITE-4199] Refactor: prepare code for nullability annotations) > The "refactor" commit is still big though (757 files changed, 7487 > insertions(+), 3827 deletions(-)) > > --- > > I tried logging findings as individual issues under CALCITE-4199, however, > that resulted in quite an unfriendly "stop doing that" feedback from Julian > (see [1]). > * Nobody expressed "please create more of these" > * It takes me time to create each ticket > * That is why I stopped creating tickets and now I perform the refactoring > as I add nullability annotations > > I would be glad if Julian admits the nullness checker finds useful bugs > (e.g. see npeTest in [2], [3], etc). > It would be great if Julian refrains from false allegations against me > (e.g. "Vladimir has been trying to fix things that aren’t broken". "He did > not leave time for anyone to review", etc, etc). > > I have provided countless samples when the nullness checker detects true > bugs, and the detected bugs are both in old and in the recently added code. > For instance, the recent discussion on SEARCH vs nullness (see [4]) was > triggered by myself trying to rebase nullness annotations on top of Calcite > 1.26. > Search/Sarg failed nullness in a non-trivial way, and I asked Julian to fix > the code. > > [1]: > > https://lists.apache.org/thread.html/rf2ee66ad385624a35829a4ce092ff13797e812e03abb80cb45b33421%40%3Cdev.calcite.apache.org%3E > [2]: > > https://lists.apache.org/thread.html/rf863922cf4418f994a123ecbbb624a74e6e375aa421a4a7812ce52b0%40%3Cdev.calcite.apache.org%3E > [3]: https://issues.apache.org/jira/browse/CALCITE-4234 > [4]: > > https://lists.apache.org/thread.html/rce6e84d0e0cd5a227d64c9f670b43650833f43e0cbccb80e04f5e0cf%40%3Cdev.calcite.apache.org%3E > > Vladimir >
