> And those arguments are currently presumed nullable? I would rather say that the current state of the code is "undetermined nullness".
Just one random example: PolarisCatalogHelpers [1]. This class has no nullability annotations, yet all of the parameters without exception are implicitly non-null. The code throws NPE if any of them is null. Alex [1]: https://github.com/apache/polaris/blob/17f28a92144e2ec00fb8151ac17a41796ebdf901/polaris-core/src/main/java/org/apache/polaris/core/catalog/PolarisCatalogHelpers.java#L34 On Mon, Jun 2, 2025 at 5:49 PM Eric Maynard <eric.w.mayn...@gmail.com> wrote: > > I think frequency is a hugely important goal, behind perhaps only accuracy > (if I see an annotation or a lack of one, can I trust it?). > > The largest group of parameters we currently have is certainly > non-annotated arguments, no? And those arguments are currently presumed > nullable? > > --EM > > On Mon, Jun 2, 2025 at 8:42 AM Alex Dutra <alex.du...@dremio.com.invalid> > wrote: > > > Hi all, > > > > > So now, the onus is on me, as the developer to write [...] > > > > But this remark is valid both ways, right? If the default is non-null > > and my method can return null, the onus is on me to annotate it with > > @Nullable; but if the default is nullable and my method cannot return > > null, then the onus is again on me to annotate it with @Nonnull. > > > > What really matters for me is: how **frequently** would I need to > > annotate things with @Nullable if we assume non-null by default, > > versus how frequently would I need to annotate things with @Nonnull if > > we assume nullable by default? > > > > From a quick glance, it seems to me we have more non-null arguments > > than nullable ones. > > > > Thanks, > > Alex > > > > On Wed, May 28, 2025 at 2:10 AM Dmitri Bourlatchkov <di...@apache.org> > > wrote: > > > > > > The thing is that IntelliJ, for example, does not infer @Nullable very > > well. > > > > > > For example: > > > > > > Map<String, String> m = new HashMap<>(); > > > private @Nullable String value() { > > > return m.get("key"); > > > } > > > > > > private void test() { > > > String value = value(); > > > if (value.length() > 3) { > > > m.put("key", value); > > > }; > > > } > > > > > > This code gives a warning on `value.length()`. However, if you remove > > > @Nullable the warning disappears. > > > > > > So the onus is on the developer to put @Nullable where relevant. Then > > IDEs > > > will help. > > > > > > If we accept that, assuming @Nonnull by default is the next logical step. > > > > > > Cheers, > > > Dmitri. > > > > > > On Tue, May 27, 2025 at 7:50 PM Michael Collado <collado.m...@gmail.com> > > > wrote: > > > > > > > The concern, for me, is whether the onus is on the developer or on the > > > > tools. For example, if I write a method: > > > > > > > > public String getTheValue() { return map.get("value");} > > > > > > > > with no annotation, the assumption is the return value of getTheValue > > is > > > > always non-null even though java.util.Map does not make any such > > guarantee. > > > > So now, the onus is on me, as the developer to write > > > > > > > > public @Nullable String getTheValue() { return map.get("value");} > > > > > > > > I can do this, but the reality is that I'm lazy and I'm probably going > > to > > > > forget and there's a high likelihood that nobody is going to call me > > out on > > > > it during code review because it's one small function in a 5,000 line > > PR :) > > > > For me, I'd much rather have the static analysis tool assume that > > anything > > > > not annotated might possibly be null so that when I write the > > following, I > > > > get a bug during the build: > > > > > > > > String foo = getTheValue(); > > > > return foo.length(); > > > > > > > > Now the tool is actually doing its job of reminding me that I'm using a > > > > value without having checked for nullability and if I want to avoid > > that > > > > warning (maybe because I'm using a special java.util.Map), I need to > > update > > > > my annotations. Given that the Iceberg codebase doesn't use these > > > > annotations, I think we'll be dealing with nullable values far more > > often > > > > than non-nullable ones. > > > > > > > > Mike > > > > > > > > On Tue, May 27, 2025 at 2:25 PM Eric Maynard <eric.w.mayn...@gmail.com > > > > > > > wrote: > > > > > > > > > Right, but I think Michael is correct that when something comes from > > > > > outside of Polaris, we have to assume it’s nullable even if it’s not > > > > > annotated as such. > > > > > > > > > > Also as we are using CDI, it may not be clear whether an > > implementation > > > > of > > > > > an interface is coming from “Polaris code” and therefore what > > assumptions > > > > > can be made about the nullability of its members. > > > > > > > > > > Given these considerations, I think developers are forced to do > > their own > > > > > due diligence for nullability when working with non-annotated > > members. If > > > > > we would go with option 1, this means developers are encouraged to > > do the > > > > > opposite and forego a nullability check on non-annotated members. > > That > > > > > seems dangerous. > > > > > > > > > > Another argument against option 1 is that for better or for worse > > most > > > > > variables in Polaris are in fact nullable. If we have the choice > > between > > > > > annotating 90% of variables and annotating 10%, I prefer the latter. > > > > > > > > > > On Tue, May 27, 2025 at 2:12 PM Dmitri Bourlatchkov > > > > > <dmitri.bourlatch...@dremio.com.invalid> wrote: > > > > > > > > > > > Yes, the idea is to simplify things by using only one of @Nullable > > > > > > and @Nonnull in Polaris code. > > > > > > > > > > > > However, I lean toward using @Nullable when necessary and assume > > > > @Nonnull > > > > > > by > > > > > > default. Again this is only for Polaris code. > > > > > > > > > > > > Cheers, > > > > > > Dmitri. > > > > > > > > > > > > On Tue, May 27, 2025 at 5:07 PM Eric Maynard < > > eric.w.mayn...@gmail.com > > > > > > > > > > > wrote: > > > > > > > > > > > > > If we've checked something is non-null or received a non-null > > value > > > > > from > > > > > > a > > > > > > > library, we can continue annotating it as @Nonnull within > > Polaris. We > > > > > can > > > > > > > also use Optional to get similar functionality to typescript's ? > > and > > > > > > handle > > > > > > > missing values that way. > > > > > > > > > > > > > > In all other cases, developers must assume any given reference > > can be > > > > > > null > > > > > > > and do their own checks. It seems to me that there is no need for > > > > > > @Nullable > > > > > > > in that case. That is, if we label everything that cannot be > > null as > > > > > > > @Nonnull and everything that can be null as @Nullable, then every > > > > > > reference > > > > > > > should be annotated as one or the other. We can simplify things > > by > > > > just > > > > > > > removing the @Nullable annotations. > > > > > > > > > > > > > > --EM > > > > > > > > > > > > > > On Tue, May 27, 2025 at 1:56 PM Dmitri Bourlatchkov < > > > > di...@apache.org> > > > > > > > wrote: > > > > > > > > > > > > > > > Your example is quite valid, Mike. In my proposal (several > > emails > > > > > ago) > > > > > > > I'd > > > > > > > > classify this as "inputs into Polaris code". > > > > > > > > > > > > > > > > However, I guess the topic of this thread is slightly > > different. > > > > The > > > > > > > > question we're trying to reach consensus on, is about what we > > > > should > > > > > > > > annotate as @Nullable in Polaris code. > > > > > > > > > > > > > > > > I'd say if a value is received from external code, which is not > > > > > > > annotated, > > > > > > > > and returned from a Polaris method without validation, and the > > > > > returned > > > > > > > > value can reasonably be null, then the Polaris method should be > > > > > > annotated > > > > > > > > as @Nullable. However, if Polaris code checks the value to be > > > > > non-null, > > > > > > > the > > > > > > > > return value need not be annotated in Polaris. > > > > > > > > > > > > > > > > Does this sound reasonable? > > > > > > > > > > > > > > > > Thanks, > > > > > > > > Dmitri. > > > > > > > > > > > > > > > > On Tue, May 27, 2025 at 4:34 PM Michael Collado < > > > > > > collado.m...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > I generally assume the other way around. Many (most) > > libraries > > > > > don't > > > > > > > > > annotate the return values of their methods, so I assume > > > > everything > > > > > > is > > > > > > > > > nullable unless specifically told otherwise. I would prefer > > > > > > everything > > > > > > > be > > > > > > > > > non-nullable unless specifically stated (the ? is the one > > thing I > > > > > > would > > > > > > > > > steal from typescript if I could), but given that library > > code > > > > > can't > > > > > > be > > > > > > > > > modified to be explicit when null return values are > > possible, I > > > > > don't > > > > > > > > think > > > > > > > > > we can make that assumption. > > > > > > > > > > > > > > > > > > Mike > > > > > > > > > > > > > > > > > > On Thu, May 22, 2025 at 10:57 AM Dmitri Bourlatchkov < > > > > > > di...@apache.org > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > My preference is for option 1 below. > > > > > > > > > > > > > > > > > > > > On Thu, May 22, 2025 at 1:53 PM Alex Dutra > > > > > > > > <alex.du...@dremio.com.invalid > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > My proposal was centered around compile-time checks and > > > > targets > > > > > > > > mostly > > > > > > > > > > > developers and contributors. I am not questioning the > > > > > usefulness > > > > > > of > > > > > > > > > > > runtime checks when these make sense. > > > > > > > > > > > > > > > > > > > > > > Maybe an example is better than a thousand words. Let's > > > > > imagine a > > > > > > > > > > > simple getOrDefault() method. Which version do you > > prefer? > > > > > > > > > > > > > > > > > > > > > > 1. Annotate only nullable items: > > > > > > > > > > > public String getOrDefault(@Nullable String s, String > > def) { > > > > > > > return s > > > > > > > > > > > == null ? def : Objects.requireNonNull(def); } > > > > > > > > > > > > > > > > > > > > > > 2. Annotate only non-null items: > > > > > > > > > > > @Nonnull public String getOrDefault(String s, @Nonnull > > String > > > > > > def) > > > > > > > { > > > > > > > > > > > return s == null ? def : Objects.requireNonNull(def); } > > > > > > > > > > > > > > > > > > > > > > 3. Annotate everything: > > > > > > > > > > > @Nonnull public String getOrDefault(@Nullable String s, > > > > > @Nonnull > > > > > > > > > > > String def) { return s == null ? def : > > > > > > > Objects.requireNonNull(def); } > > > > > > > > > > > > > > > > > > > > > > Many places in Polaris are using option 3, which is too > > > > verbose > > > > > > and > > > > > > > > > > > leads to visual fatigue. What I was suggesting to the > > > > community > > > > > > is > > > > > > > to > > > > > > > > > > > adopt option 1, that reduces the visual clutter and also > > > > > assumes > > > > > > > > > > > non-null by default. > > > > > > > > > > > > > > > > > > > > > > (You will notice that I added a runtime check to all > > three > > > > > > > versions.) > > > > > > > > > > > > > > > > > > > > > > Hope that helps to clarify the discussion. > > > > > > > > > > > > > > > > > > > > > > Alex > > > > > > > > > > > > > > > > > > > > > > On Wed, May 21, 2025 at 8:54 PM Yufei Gu < > > > > flyrain...@gmail.com > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > In my opinion, assuming everything is nullable by > > default > > > > > > isn't > > > > > > > > the > > > > > > > > > > > > > best approach for writing robust code. I believe a > > bias > > > > > > towards > > > > > > > > > > > > > non-nullness leads to more reliable systems. > > > > > > > > > > > > > > > > > > > > > > > > I agreed with the intention, but am concerned that > > assuming > > > > > > > > > everything > > > > > > > > > > is > > > > > > > > > > > > non-nullness may discourage null-checking, which is > > > > > problematic > > > > > > > as > > > > > > > > > > > runtime > > > > > > > > > > > > null-checking isn't a thing. > > > > > > > > > > > > > > > > > > > > > > > > Yufei > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, May 21, 2025 at 9:52 AM Alex Dutra > > > > > > > > > > <alex.du...@dremio.com.invalid > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hi Eric, > > > > > > > > > > > > > > > > > > > > > > > > > > You're right that annotations don't change the > > bytecode > > > > at > > > > > > > > runtime. > > > > > > > > > > > > > Their real value comes during compilation, as many > > static > > > > > > > > analysis > > > > > > > > > > > > > tools use them to flag potential issues. They can > > even > > > > > cause > > > > > > > > build > > > > > > > > > > > > > failures depending on how you configure them. My IDE > > > > > > (IntelliJ) > > > > > > > > > > > > > frequently warns me when I forget to handle a > > potential > > > > > NPE; > > > > > > if > > > > > > > > > > you're > > > > > > > > > > > > > not seeing similar feedback, it might be worth > > checking > > > > > your > > > > > > > IDE > > > > > > > > > > > > > settings. > > > > > > > > > > > > > > > > > > > > > > > > > > While the annotations are primarily for compile-time > > > > > checks, > > > > > > > that > > > > > > > > > > > > > doesn't mean we can't also incorporate runtime > > checks. We > > > > > > > should > > > > > > > > > aim > > > > > > > > > > > > > to include these whenever this makes sense, for > > example > > > > by > > > > > > > using > > > > > > > > > > > > > Guava's Preconditions. This is especially useful if > > we > > > > > can't > > > > > > > > > > guarantee > > > > > > > > > > > > > that a method parameter, for instance, will never be > > > > null, > > > > > > > > because > > > > > > > > > > > > > it's being provided by some external system. > > > > > > > > > > > > > > > > > > > > > > > > > > In my opinion, assuming everything is nullable by > > default > > > > > > isn't > > > > > > > > the > > > > > > > > > > > > > best approach for writing robust code. I believe a > > bias > > > > > > towards > > > > > > > > > > > > > non-nullness leads to more reliable systems. > > > > > > > > > > > > > > > > > > > > > > > > > > I am also a big fan of Optional and think we should > > > > strive > > > > > to > > > > > > > use > > > > > > > > > it > > > > > > > > > > > > > as much as possible. That said, it's not always > > possible, > > > > > > > > > especially > > > > > > > > > > > > > if you are implementing a third-party interface that > > > > > doesn't > > > > > > > use > > > > > > > > > it. > > > > > > > > > > > > > Using Optional in class fields and method parameters > > is > > > > > also > > > > > > > > > > > > > controversial: Optional was designed primarily as a > > > > signal > > > > > > from > > > > > > > > the > > > > > > > > > > > > > callee to the caller, to signify: "no result". In > > other > > > > > > words, > > > > > > > > its > > > > > > > > > > > > > main purpose is to clarify method return types. This > > post > > > > > on > > > > > > > > Stack > > > > > > > > > > > > > Overflow by Brian Goetz is worth reading: [1]. > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > > > > > > > > > > > Alex > > > > > > > > > > > > > > > > > > > > > > > > > > [1]: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://stackoverflow.com/questions/26327957/should-java-8-getters-return-optional-type/26328555#26328555 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, May 21, 2025 at 4:37 PM Eric Maynard < > > > > > > > > > > eric.w.mayn...@gmail.com > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks for bringing this up as I’ve been confused > > by > > > > > this a > > > > > > > few > > > > > > > > > > > times. > > > > > > > > > > > > > > > > > > > > > > > > > > > > Before Polaris I hadn’t really encountered these > > > > > > annotations > > > > > > > > and > > > > > > > > > I > > > > > > > > > > > was > > > > > > > > > > > > > > surprised to learn they don’t “do anything” — that > > is, > > > > > > there > > > > > > > is > > > > > > > > > no > > > > > > > > > > > > > > additional safety you get at runtime when a null > > value > > > > is > > > > > > > > passed > > > > > > > > > > > into a > > > > > > > > > > > > > > parameter marked non-null. Similarly nothing > > enforces > > > > > that > > > > > > > you > > > > > > > > > > handle > > > > > > > > > > > > > null > > > > > > > > > > > > > > values when something is annotated as nullable. > > > > > > > > > > > > > > > > > > > > > > > > > > > > For that reason, I tend to assume everything is > > > > nullable > > > > > > > > > regardless > > > > > > > > > > > of > > > > > > > > > > > > > > annotation and I would be in favor of standardizing > > > > > around > > > > > > > that > > > > > > > > > > > > > assumption. > > > > > > > > > > > > > > Iff something is annotated as Non-null a developer > > > > should > > > > > > > feel > > > > > > > > > safe > > > > > > > > > > > > > > skipping a check for null, but otherwise they > > should > > > > > handle > > > > > > > > null. > > > > > > > > > > > > > > > > > > > > > > > > > > > > I am a big fan of Optional and of trying to reduce > > the > > > > > > usage > > > > > > > of > > > > > > > > > > null > > > > > > > > > > > as > > > > > > > > > > > > > > much as possible though. > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, May 21, 2025 at 3:02 PM Alex Dutra > > > > > > > > > > > <alex.du...@dremio.com.invalid > > > > > > > > > > > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > A while ago, we had a discussion regarding which > > > > > nullness > > > > > > > > > > > annotations > > > > > > > > > > > > > > > to use and whether we should consider favoring > > > > non-null > > > > > > by > > > > > > > > > > > default. I > > > > > > > > > > > > > > > would like to revive that discussion. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > We are currently using the `jakarta.annotation` > > > > package > > > > > > > > > > > consistently, > > > > > > > > > > > > > > > but the defaults are not clear: should we > > consider > > > > > > > everything > > > > > > > > > as > > > > > > > > > > > > > > > non-null by default and only annotate the > > nullable > > > > > > things, > > > > > > > or > > > > > > > > > the > > > > > > > > > > > > > > > other way around? Some classes are cluttered with > > > > both > > > > > > > > > > annotations, > > > > > > > > > > > > > > > which seems unnecessary and confusing. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I would personally be in favor of considering > > > > > everything > > > > > > as > > > > > > > > > > > non-null by > > > > > > > > > > > > > > > default. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Please let me know your thoughts. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Alex > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
