Like Stamatis, I have quickly gone over the changes, and I find that the code
is not significantly different, and overall it is an improvement. Thanks for
making this change, Vladimir.
PolyNull is difficult to understand, and I don’t think we should require
developers to understand it. Therefore, when it is used in the signature of
public methods (i.e. methods that have javadoc), the javadoc should explain the
nullness behavior in words. For example, in CalciteConnectionConfig:
/** Returns the value of {@link CalciteConnectionProperty#FUN}.
* If {@code defaultOperatorTable} is not null, the return will never be
null. */
<T> @PolyNull T fun(Class<T> operatorTableClass, @PolyNull T
defaultOperatorTable);
A few things need to be fixed:
* Remove the changes to 2020-10-06-release-1.26.0.md
* Add descriptions to javadoc for PolyNull.
* Remove Wrapper.unwrapOrThrow. We did not reach consensus in
https://issues.apache.org/jira/browse/CALCITE-4311
<https://issues.apache.org/jira/browse/CALCITE-4311> and Vladimir
said that he would proceed without it.
+1 to merge when those things are fixed.
Julian
> On Nov 23, 2020, at 1:31 AM, Stamatis Zampetakis <[email protected]> wrote:
>
> 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
>>
