Thanks for the review, Julian.
Are you willing to help with fixing the bits that you identified?
Could you suggest timeframes when you can implement that?
Julian>PolyNull is difficult to understand
I agree PolyNull is not the most obvious thing, however, "arrays vs
annotations" is even harder :)
I think I can add a couple of words on PolyNull to develop/index.md.
Julian>* Remove the changes to 2020-10-06-release-1.26.0.md
Can you please clarify what is wrong with that change?
Do you think Calcite 1.26 is a good release which people should use in
production systems like 1.25 and other versions?
Do you think people should prefer upgrading to 1.26 as soon as possible?
Do you have better wording for the warning?
Julian> * Add descriptions to javadoc for PolyNull. signature of public
methods (i.e. methods that have javadoc),
There are ~18 methods that have PolyNull and javadoc at the same time.
However, I'm not sure nullness is really description is really needed.
You mention
/** 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);
I believe the following would be better:
/**
* Returns connection operator table or defaultOperatorTable if missing.
* @param defaultOperatorTable default operator table
* @param <T> the type of the operator table
* @return connection operator table or defaultOperatorTable if missing
* @see CalciteConnectionProperty#FUN
**/
<T> @PolyNull T fun(Class<T> operatorTableClass, @PolyNull T
defaultOperatorTable);
In other words, I agree PolyNull is slightly more complex than regular
Nullable.
However, I believe javadoc update can happen after the change is merged,
and sometimes
the behavior is much more important than nullability
For instance, CalciteConnectionConfigImpl has the following implementation:
if (fun == null || fun.equals("") || fun.equals("standard")) {
return defaultOperatorTable;
}
Should those "empty string" and "standard" be documented in the interface?
I would say it is not the most obvious thing when one looks into the
following javadoc
/** Returns the value of {@link CalciteConnectionProperty#FUN}.
* If {@code defaultOperatorTable} is not null, the return will never be
null. */
Julian>* Remove Wrapper.unwrapOrThrow.
The added Wrapper.unwrapOrThrow is API.Status.INTERNAL. What is the problem
with adding such a method?
unwrapOrThrow is used 16 times, and allows to have concise code and produce
meaningful exception messages at the same time.
I can easily inline the methods, however, it would make all the 16 usages
more verbose.
For instance the following code
final String timeZone = filter.getCluster().getPlanner().getContext()
.unwrapOrThrow(CalciteConnectionConfig.class).timeZone();
would become
Wrapper wrapper = filter.getCluster().getPlanner().getContext();
final String timeZone =
requireNonNull(wrapper.unwrap(CalciteConnectionConfig.class),
() -> "Can't unwrap " + CalciteConnectionConfig.class + " from "
+ wrapper).timeZone();
That is way harder to both read and write.
There are similar helper methods in NonNullableAccessors which is used 20
times, and SqlNonNullableAccessors is used 39 times.
If you think you have a much better solution, I do not hold you. Please go
ahead and implement it.
In the mean-time, I suggest we merge the code with unwrapOrThrow, etc, etc,
and later you (or someone else) can suggest and implement your own
way provided it is even better than the current one.
Vladimir
