>From the peanut gallery... I have been spending my "free" hours adding checkerframework nullness typing to Beam and reliably finding bugs. It is super valuable. It is not at all surprising that Vladimir has uncovered bugs. The likelihood of a Calcite-sized project successfully defending against nullability errors via any method other than via a sound type system is zero. The only way to eliminate NPEs from Calcite/Beam/etc is by adding such a type system.
When it does not uncover bugs, it often makes code more readable. Some static analyses cannot grok good code well enough to accept it. That is not the case for checkerframework in most cases, in my experience. They have covered the common OK cases like "this Set contains the keys for that Map" or "this can only be null if that other thing is null". Since it is a type system, it is trivial to understand, doubles as good documentation, and has no weird behaviors like spotbugs and other heuristic approaches. It is a valid point that any place a user can pass things in they might violate the null-safe type system. This is not a new problem, but a subset of your current problem. FWIW it would make my life in Beam better if Calcite's public APIs had accurate nullness types. I can see that there is more to this discussion than just "should we get Calcite to use a nullness type checker" but if you could limit scope to that question, I would emphatically say "yes, please". Kenn p.s. <rant>The unchecked assumption that anything is nullable by default is an obsolete concept of yesteryear, and a solved problem. The cost of the "billion dollar mistake" was a massive underestimate. For a time, nullness safety was confined to "advanced" languages and was not available to those working in more widespread industrial languages. With Java's pluggable type systems and the checkerframework it is finally available to "everyone". There is no excuse (other than lack of time to do the work) remaining to ever have an NPE under any circumstances. It is more justifiable to have SEGV-vulnerable code (for perf reasons) than NPE-vulnerable code, IMO.</rant> On Sat, Sep 5, 2020 at 4:45 PM Vladimir Sitnikov < [email protected]> wrote: > Hey Julian, > > I can imagine it might be unexpected (and/or fearful) to see that many > issues / changes out of a blue. > I do not expect that you rush into review right away (or even spent your > time on that!). > > However, I do value your feedback, and it is indeed helpful. No jokes, it > was helpful, and I was surprised for having feedback that early. > > Let me add some clarifications: > 1) I create tickets under a single parent, so it helps to keep them > together and/or close in a bulk. > 2) Sometimes I find "interesting" code (e.g. dead code?), and I create a > ticket just in case to manage the workqueue > 3) Many tickets might even be closed later for any reason (e.g. > https://issues.apache.org/jira/browse/CALCITE-4231 ). > I won't complain like "I want to make a code change for the ticket". For > instance, if 4231 is dead code, and we agree to keep it, ok. If we agree to > deprecate&remove, > that is fine as well. > 4) I do my best to incorporate feedback, and I believe I made the changes > in 4226 and 4217 in line with your feedback. > 5) There are cases where I need more time to settle on the solution and > figure out the way to proceed (e.g. statistics, or even > 4214 RelDataType#getFamily). Even though draft PRs are there, I'm not sure > they are all good, so I do not commit them yet. > > >And the moment I look away, he will commit them > > It is really sad to hear that. In any case, even if I type the wrong > command and push something unexpected, the option to revert / force-push is > always there. > There's no way I push a change like "add nullability annotations" without a > discussion. > Note: the change is not ready yet, and I believe there's nothing to discuss > yet. We don't know the amount of the changes, we don't know how it looks, > and so on. > > Of course, early feedback is always welcome. > > ---- > > Julian>in my opinion doesn’t improve the project one iota. > > It is sad to hear that as well. > "[CALCITE-4228] FlatLists.Flat6List#append" is a true bug. > There's discussion pending on "statistics nullability". I hope we could > figure out the way to make it into a better API for the users and make the > core stronger. > > Here's a PR that shows Calcite core fails with NPEs if only statistic > callbacks respond with nulls (which is explicitly allowed by the javadocs > in core, and it was you who insist that "unknowns" should be returned as > null): > https://github.com/apache/calcite/pull/2136/files > > Traits / TraitSet are not null-ready, so we might need to discuss if nulls > should be allowed there at all. > > The above alone does sound like a non-trivial improvement to me. > > Julian>CALCITE-4226>. It works fine. There are no bugs. Yet he still > commits a change. > > Here's a test: > > @Test void npeTest() { > RelBuilder.create(config().build()) > .scan("EMP") > .fields(Mappings.target(Arrays.asList(2, 4), 10)); > } > > The current Calcite fails as follows: > > java.lang.NullPointerException > at org.apache.calcite.tools.RelBuilder.fields(RelBuilder.java:544) > at org.apache.calcite.tools.RelBuilder.fields(RelBuilder.java:566) > at org.apache.calcite.test.RelBuilderTest.npeTest(RelBuilderTest.java:228) > > It does look like a bug to me. > I believe RelBuilder#fields should be user-friendly public API, so it > should not just throw NPE. > At least it should clarify what went wrong. > > That is why I added a an extra function for Mappers that would make the > message easier to understand. > > --- > > Julian> CALCITE-4217> to the list of changes that Vladimir has committed > despite my clear objections. > > You asked to keep "crosstype is not struct" behavior. I believe I did > exactly that. > > However, it is clear you still have objections :( Could you please clarify > what are they? I truly do not understand what is wrong. > The bug was "getFieldCount was not working", I fixed that, and I kept > everything else as is. > > The change in 4217 was to allow clients to call getFieldCount() > Note: the clients could use getFieldList().size() as a workaround, however, > it does look awkward that a trivial getFieldCount() (which is in > RelDataType interface) fails to work. > Of course, users can figure out the workaround, but why should it be broken > like that? > > Vladimir >
