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
> > > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
>
