Hello, I went fast over the diff and overall the changes do not make the code that different than before. Moreover, the refactoring for preparing for nullable annotations seems beneficial in any case (finalizing instance variables, adding requireNonNull for enforcing fail fast behavior, removing usages of raw types, etc.). The checker can find some bugs so I think it will be nice to have an extra layer of checks in the CI. The 10min might be a bit long compared with our unit tests that are around ~5min but we can give it a try and if we are not happy with it we can possibly modify the frequency that these checks are performed.
Although, I didn't do a line-by-line review, I am rather positive with merging this to master. Best, Stamatis On Fri, Nov 20, 2020 at 1:23 PM Vladimir Sitnikov < [email protected]> wrote: > Fan Liya>However, I think it would be nice if we could document it > somewhere, so > Fan Liya>other developers will follow the convention without being > confused. > > The PR does include documentation update: > > https://github.com/vlsi/calcite/blob/checkerframework/site/develop/index.md#null-safety > > Fan Liya>I mean that a large PR requires a long time to review. > > Do you commit to reviewing the change? > How much time would it take you to review the change? > > As I said, the PR was there for several months, and I do NOT believe adding > two or six months more would make a difference. > > Fan Liya>During this time, you have to resolve conflicts many times, which > is a > Fan Liya>large amount of effort. > > The added nullability annotations have minimal impact on Calcite API and > its behavior. > All the existing tests pass, and I change only a few test files. > > Of course, I do not like the idea of rebasing PR again and again. > > Just as an example, Calcite 1.25 was released with a significant > backward-incompatible change: > > 998cd83eb 2020-07-08 Julian Hyde [CALCITE-3923] Refactor how planner rules > are parameterized > 198 files changed, 9018 insertions(+), 5503 deletions(-) > > The refactor alone was bigger than nullability PR if we compare the number > of added and removed lines. > Just to remind, "prepare for nullability" is (757 files changed, 7487 > insertions(+), 3827 deletions(-)) > > Vladimir >
