[CALCITE-7736] Replace the Checker Framework with NullAway and JSpecify - #5213
Draft
vlsi wants to merge 65 commits into
Draft
[CALCITE-7736] Replace the Checker Framework with NullAway and JSpecify#5213vlsi wants to merge 65 commits into
vlsi wants to merge 65 commits into
Conversation
The Checker Framework verified nullness through a Gradle plugin of its own, a set of `.astub` files that patched the nullness of the JDK and of third-party libraries, and two dedicated CI jobs. NullAway runs as an Error Prone check instead, so it needs no separate plugin and no stub files: it ships nullness models for the JDK and for popular libraries, and JSpecify supplies the annotations. Build: * drop the `org.checkerframework` plugin and its configuration block, and delete the 48 `.astub` files * add `com.uber.nullaway:nullaway` to the `errorprone` configuration and configure it for JSpecify, including the experimental generics support (`JSpecifyExperimental`, `HandleWildcardGenerics`, `JSpecifyJDKModels`, `WarnOnGenericInferenceFailure`) * replace `org.checkerframework:checker-qual` with `org.jspecify:jspecify` * raise Error Prone to 2.50.0 and the Error Prone plugin to 5.1.0, which NullAway requires NullAway is an error in the projects listed in `nullawayProjects` and is off elsewhere, so a nullness problem fails one CI job rather than every test job. CI: * drop the two `CheckerFramework` jobs * fold nullness verification into the `errorprone` job, which moves to JDK 21 because Error Prone 2.43 and later require it This commit only moves the tooling; the source still carries Checker Framework annotations and is migrated by the commits that follow.
… to JSpecify A purely mechanical rename of `org.checkerframework.checker.nullness.qual.Nullable` to `org.jspecify.annotations.Nullable`, and of the matching `NonNull`. Both annotations are `TYPE_USE`, so every existing position stays valid and no annotation moves. Autostyle rewrites either annotation to its JSpecify counterpart from now on, alongside the rule that already rewrote the jsr305 ones. The Checker Framework annotations that JSpecify does not define -- @polynull, @MonotonicNonNull, @RequiresNonNull and the rest -- are still here and are migrated by the next commit.
…fies
`@DefaultQualifier(NonNull, {FIELD, PARAMETER, RETURN})` is how the Checker
Framework said "types are non-null unless annotated". JSpecify says it with
`@NullMarked`, which covers every type position rather than three of them.
Only `calcite-linq4j` and `calcite-core` are verified, so only their main
packages get the annotation: `@NullMarked` claims that a package is fully
annotated, and in an unverified module nothing backs that claim. The 23
`package-info.java` files that carried `@DefaultQualifier` are converted, and
the remaining packages of those two modules get the annotation.
`babel`'s test `package-info.java` also carried `@DefaultQualifier`. It loses
the annotation rather than gaining `@NullMarked`, because `checker-qual` is
gone and test code is not verified.
NullAway skips a package that is not `@NullMarked`, so a missing annotation
costs coverage without saying a word. `LintTest.testLintNullMarked` walks the
source roots in `NULL_MARKED_ROOTS` and fails when a package there has no
`package-info.java`, or has one that does not declare `@NullMarked`. Widen that
list together with `nullawayProjects`.
…y does not define JSpecify defines @nullable, @nonnull and @NullMarked, and nothing else. The remaining Checker Framework annotations either move to a Calcite-owned equivalent or go away. Adds `org.apache.calcite.linq4j.annotations` with @contract, @MonotonicNonNull, @RequiresNonNull, @EnsuresNonNull and @EnsuresNonNullIf. NullAway matches these by the last component of their name rather than by their package, so Calcite declares its own and takes no dependency on the checker: * `ContractUtils.hasSimpleNameContract` compares the simple name * `Nullness.isMonotonicNonNullAnnotation` tests `endsWith(".MonotonicNonNull")` * the field-contract handlers pass `exactMatch=false` to `NullabilityUtil.findAnnotation`, which also compares the suffix Moved to that package: @MonotonicNonNull (32), @RequiresNonNull (21), @EnsuresNonNull (23) and @EnsuresNonNullIf (14). The two @EnsuresNonNull that named a parameter rather than a field are dropped, along with the four @EnsuresNonNullIf whose expression was a method call: NullAway supports fields only. @polynull (300) becomes @nullable. It says the result is null exactly when the argument is null, which @nullable weakens to "may be null"; the next commit restores the other half with @contract. Dropped, having no equivalent and no NullAway counterpart: @pure (81), the initialization annotations (80), @KeyFor and @UnknownKeyFor (18), @covariant (10), @HasQualifierParameter and @minlen. NullAway checks initialization on its own, so the receiver parameters that existed only to carry @UnderInitialization are removed with them. `<@nullable R>` becomes `<R extends @nullable Object>`. The Checker Framework reads an annotation on a type parameter declaration as a bound on the lower bound, so `<@nullable R>` there means R *must* be nullable; JSpecify tracks upper bounds only and can say no more than "may be". 215 @SuppressWarnings that named Checker Framework message keys such as `argument.type.incompatible` become "NullAway". `Nullness.castNonNull` keeps working as NullAway's `CastToNonNullMethod`; it loses @pure and its parameter-naming @EnsuresNonNull, and its blanket suppression narrows to the one method that needs it.
@polynull said the result is null exactly when the argument is null. The previous commit weakened it to @nullable, which makes every caller treat the result as nullable even when it passed a non-null argument. @contract states the direction that matters at a call site: @contract("!null, _ -> !null") public static @nullable Integer plus(@nullable Integer b0, int b1) NullAway is configured with `CheckContracts=true`, so it verifies each clause against the method body rather than trusting it, and none of the 108 clauses here is rejected. Not every @polynull converts. A clause describes arguments, so it cannot describe a receiver parameter, and it cannot describe a varargs method, whose call sites pass a different number of arguments. Nor does it reach a type argument such as `Enumerable<@nullable T>`, where the element rather than the result is polymorphic. Those keep the plain @nullable. Passing a clause whose length does not match the call crashes NullAway with an IndexOutOfBoundsException from `ContractHandler.onDataflowVisitMethodInvocation`, which reads arguments by the antecedent's length without checking the arity it was given. It validates that on declarations but not at call sites; worth reporting upstream.
…ramework inferred The two tools default an unwritten type parameter bound in opposite directions. The Checker Framework's CLIMB-to-top rule gives implicit bounds the top qualifier, so `<T>` there means `<T extends @nullable Object>`. JSpecify fills in `Object`, which under `@NullMarked` is non-null, and its user guide says as much: "`<E>` means `<E extends Object>` and that means it is not `@Nullable`". So every unbounded type parameter silently changed meaning. Calcite relied on the Checker Framework reading, for instance here: public interface SqlVisitor<R> { ... } public class SqlShuttle extends SqlBasicVisitor<@nullable SqlNode> { ... } public abstract <R> R accept(SqlVisitor<R> visitor); `SqlShuttle` is a `SqlVisitor<@nullable SqlNode>` and was passed to `accept` with no suppression, which only typechecks if R admits a nullable argument. Writes the bound out at 61 declarations: the Rex and Sql visitors and the 23 `accept` overrides, `Pair` and its factories, `PairList`, `ConsList`, `FlatLists`, `Holder`, `ImmutableNullableList`, `TryThreadLocal`, `Util.transform`, and the linq4j types `Enumerable`, `Enumerator`, `Queryable`, `Function0`, `Function1`, `Function2` and `Ord`. The erasure is unchanged, so this is binary compatible. This accounts for most of what NullAway reported: 1126 errors down to 576 in `calcite-core`. `Pair.of` alone was worth 132 -- its class already had the bounds, but a static factory declares type parameters of its own.
`Visitor<R>` computes a value by walking a node tree, and there is nothing to compute for an empty subtree: `Expressions.acceptNodes` returns the result of the last node, or null when the list is empty, and `VisitorImpl` returns a bare `null` for a `FunctionExpression` with no body and for a `GotoStatement` with no expression. `Visitor.visit` and `Node.accept(Visitor)` now say so, and the type variable takes the nullable bound the Checker Framework used to infer for it. Under the Checker Framework this was `VisitorImpl<@nullable R>`, which forced R to be nullable; JSpecify tracks upper bounds only, so the annotation moves to the result. `UseCounter` extended `VisitorImpl<Void>`, a type whose only value is null. It now extends `VisitorImpl<@nullable Void>`, like `MayThrowVisitor`. Nothing outside `org.apache.calcite.linq4j.tree` implements `Visitor` or calls the `accept` overload that takes one.
The seedless `aggregate` starts from the first element and returns null when there is none. The `min` and `max` overloads that call it returned a non-null type anyway, so an empty sequence produced a null the signature ruled out. Eight of them now say `@Nullable`; the other overloads either seed the accumulator or already wrap the call in `requireNonNull`, and are unchanged. Four `min` and `max` overloads were already annotated, so this makes the family consistent. `aggregate` declares its accumulator `Function2<@nullable TSource, TSource, TSource>`, and the reducers it is called with genuinely handle a null accumulator -- every `MIN` and `MAX` constant in `Extensions` opens with `v1 == null`. Their declarations now match; the `SUM` constants do not test for null and keep the non-null accumulator.
`aggregate(source, seed, func)` is the reduce that takes a starting value, and `min` and `max` start it at null. Its accumulator type variable was non-null, so the seed, the reducer and the result all disagreed with the call. `TAccumulate` and `TResult` take the nullable bound the Checker Framework used to infer for them. The reducer parameter becomes `Function2<TAccumulate, TSource, ? extends TAccumulate>`: the constants in `Extensions` accept a null accumulator but never produce one, and the wildcard is what lets a reducer with a non-null result feed a nullable seed. Three more `min` overloads return null for an empty sequence and now say so, and `long min(Enumerable, LongFunction1)` wraps the call in `requireNonNull`, which is what the other overloads that unbox the accumulator already do. `EqualityComparer<T>` takes the nullable bound as well; `Functions` builds comparers over nullable elements. The two `aggregate` bodies carry a NullAway suppression. Assigning the result of a call returning `? extends TAccumulate` to a `TAccumulate` local makes NullAway report the local as @nullable, even on `return result;` where the local's type is the return type. Spelling the type argument exactly reports nothing, so the wildcard is what triggers it.
NullAway reports a `castToNonNull` whose argument it can already prove non-null, which is how it flags a cast that has stopped earning its place. Four of them in `EnumerableDefaults`: `curAccumulator` is assigned from `accumulatorInitializer.apply()` a few lines above each use, and `outerValue` from `outerValues.get(i)`.
Four places read a value the JDK is entitled to leave null. The Checker Framework knew about the first two from `InvocationHandler.astub`, which this migration deleted; NullAway's own JDK models say the same thing. * `InvocationHandler.invoke` receives a null `args` array when the proxied method declares no parameters. `Compatible` indexed it, and `FunctionExpression` forwarded it to a varargs call that requires an array. * `Primitive.asList` adapts a primitive array, whose elements box to a non-null value, so the `Array.get` result is cast rather than checked. * `EnumerableDefaults.takeTopN` looked up the key it had just read from `lastKey()`, and suppressed the finding on the declaration while dereferencing the value on the next line. It now uses `requireNonNull`.
`MergeUnionEnumerator.initEnumerators` carried `@RequiresNonNull("inputs")`.
`inputs` is a final field assigned in the constructor; the annotation existed
to get the Checker Framework's initialization checker past a call made from
that constructor. NullAway does its own initialization analysis, so the
annotation only made callers prove something already guaranteed, and the
suppression that silenced it goes with it.
`DefaultEnumerable.aggregate` carried `@Contract("!null, !null -> !null")`,
generated when `@PolyNull` was replaced. The original was polymorphic in the
accumulator function as well, so a non-null seed implied a non-null result. The
function may now return null, which makes the clause claim more than the method
delivers -- and NullAway, running with `CheckContracts`, said so.
`Linq4j.SingletonNullEnumerator` yields exactly one element and that element is null. `Functions.Ignore` implements the function interfaces by returning null from every `apply`. Both are meaningful only when their type argument is instantiated nullable. The Checker Framework could demand that: `<@nullable E>` on a type parameter declaration constrains the lower bound, so E had to be nullable. JSpecify tracks upper bounds only, and `<E extends @nullable Object>` says "may be" rather than "must be". Neither class is public, and both carry a suppression naming the reason.
`MemoryFactory` keeps a fixed-size window of rows for MATCH_RECOGNIZE, backed by a `@Nullable Object[]`. `MemoryEnumerator` pads it with `add(null)` once the input runs out, so that the last rows can still be read with their following context, and a slot that has not been written yet reads back as null either way. `add` takes `@Nullable E`, `Memory.get` returns it, and both type parameters take the nullable bound.
An outer join emits a row where one side is missing, and the merge and correlate joins build that row from a null. The types said otherwise. * `ExtendedEnumerable.correlateJoin` takes `Function2<TSource, ? super @nullable TInner, TResult>`. Its own javadoc already said "for semi/anti join inner argument is always null", and `EnumerableDefaults.correlateJoin` already declared the parameter that way. * `CartesianProductJoinEnumerator` accepts `Enumerator<? extends @nullable TInner>`, which is what a left join hands it as the single null inner row; its result selector already took `@Nullable TInner`. * `Linq4j.enumerator(Collection)` and `Linq4j.IterableEnumerator` take the nullable bound, so that null row can be enumerated at all. * Merge join widens its result selector before handing it to `nestedLoopJoin`, which serves right and full joins too and so requires one that tolerates a null left row. Merge join rejects those join types up front. `IterableEnumerator.moveNext` carries a NullAway suppression: assigning `Iterator<? extends T>.next()` to a field of type `T` is reported as assigning @nullable to @nonnull, the same limitation the seeded `aggregate` hits. `WrapMap.get` and `remove` take a nullable key, as `Map` requires, and wrapping one throws. That is what the map did before it was annotated; `put` already carried the same suppression. With this, NullAway reports nothing in calcite-linq4j.
`RexVisitorImpl`, `RexBiVisitorImpl` and `SqlBasicVisitor` are the traversals that subclasses extend and override where they care; every method they define returns null. That is meaningful only when the result type is instantiated nullable, which JSpecify cannot require: it tracks upper bounds, so `<R extends @nullable Object>` says "may be" rather than "must be". The Checker Framework said it with `<@nullable R>`, which constrains the lower bound. Same treatment as `Linq4j.SingletonNullEnumerator` and `Functions.Ignore`.
…Void A visitor that computes nothing was written `RexVisitorImpl<Void>`. `Void` has null as its only value, so a non-null `Void` is a type nothing inhabits, and the traversal returns exactly the null it rules out. 68 sites across `RexVisitorImpl`, `RexVisitor`, `SqlBasicVisitor`, `SqlVisitor` and `RexBiVisitorImpl` now say `@Nullable Void`, including the parameters and fields that hold such a visitor.
NullAway reports a `castToNonNull` whose argument it can already prove non-null, which is how it flags a cast that has stopped earning its place. 46 of them across 20 files, most in the `FlatLists` and `PairLists` constructors, where the elements arrive as non-null parameters.
…ides `java.util.List` declares `<T extends @nullable Object> T[] toArray(T[] a)`: the type variable carries the nullability and the array argument is required. The six `FlatNList` overrides declared `<T2> @nullable T2[] toArray(T2 @nullable [] a)`, which disagreed on all three counts, and then had to cast the argument back to non-null before reading its length. The six `Object[] toArray()` overrides carried a receiver parameter, `Flat1List<@nullable T> this`, which is how the Checker Framework said "only when T is nullable". JSpecify has no such form, and the annotation is what crashed NullAway when the migration generated a `@Contract` for these methods. `ComparableListImpl.toArray` casts what it delegates to. NullAway's JDK model reports `Collection.toArray()` as returning `@Nullable Object[]` at a call site while requiring an override to return `Object[]`, so a class that both implements `List` and calls `toArray` on another list has no signature that satisfies both.
`PairList` is declared `<T extends @nullable Object, U extends @nullable Object>`, but the classes in `PairLists` that implement it, and the `MapEntry` they hand back, declared plain `<T, U>`. A nullable type argument was therefore rejected at every one of them. `MutablePairList` keeps both halves of every pair in one `List<@nullable Object>`, so the element type cannot carry the nullability of T and of U separately, and reading a slot back produced a `@Nullable` value at 26 call sites. They now go through one `element` helper that explains the packing and carries the suppression, rather than each site arguing the point. `backingList` answers with `Arrays.asList` and `Collections.emptyList` instead of `ImmutableList`, which rejects a `@Nullable` element type. The array-backed implementation already answered that way.
`select` maps each element through a function, and the function is free to return null: `SqlFunctions` does exactly that when it projects a column that may be absent. The `<TResult>` type parameter said otherwise on `ExtendedEnumerable`, `ExtendedQueryable`, `QueryableFactory`, `EnumerableDefaults` and the classes that implement them, so a nullable type argument was rejected at the call. Adds the nullable bound to every `<TResult>` and `<TSource, TResult>` method in linq4j, and to the two overrides of those methods in core.
A SQL array or multiset may hold nulls, and an outer join hands the adapter a null collection outright, so the types these helpers work with have to say so. * `Functions.compareLists` and `compareMaps` accept `? extends @nullable Object` * `Linq4j.product` gives its element type a nullable bound * the two outer-join adapters in `SqlFunctions` take `@Nullable List<@nullable Object>` rather than `List<Object>`, which is what they were already handed * `mapFromEntries` builds a `Map<@nullable Object, @nullable Object>` rather than a raw one * `IS_JDK_8` reads `java.version`, which is always set, through `requireNonNull` `ArrayCartesianProductEnumerable` calls `toArray` through a lambda rather than a method reference, because the JDK model reports one signature at a call site and requires another in an override.
… from Three findings turned out to be NullAway defects rather than Calcite ones, and are now filed upstream. The places that work around them say which: * uber/NullAway#1726, a `@Contract` clause whose antecedent arity does not match the call crashes the analysis. The rule in the contributing guide says so. * uber/NullAway#1727, a call returning `? extends T` reads as `@Nullable` when `T` has a nullable upper bound. Both `EnumerableDefaults.aggregate` overloads. * uber/NullAway#1728, `Collection.toArray()` reports one signature at a call site and requires another in an override. `FlatLists` twice, and the lambda in `SqlFunctions` that replaced a `List::toArray` method reference.
…getOrDefault `getOrDefault(key, new BitSet())` cannot return null: the maps hold non-null `BitSet` values and the default is non-null. NullAway reported all 16 calls as nullable anyway, because the fields are declared `HashMap`, which overrides `getOrDefault`, and the model of `Map.getOrDefault` is not carried over to the override. A field declared `Map` reports nothing, and so does `TreeMap`, which inherits the method rather than overriding it. One `edgesOf` helper reads the map with `get` and answers an empty set, which depends on no model and says at one place what the 16 call sites were each implying. Reported as uber/NullAway#1729, see nullaway-bugs/jdk-model-shadows-inherited-library-model.md.
… elements `ImmutableNullableList` exists to hold nulls, and `Pair` is declared `<T1 extends @nullable Object, T2 extends @nullable Object>`. Their static factories said otherwise: a static method declares type parameters of its own, and these had none, so a nullable element was rejected at every call. * `ImmutableNullableList`, its three `copyOf` overloads and `builder`. The inner `Builder` already had the bound, which is why `builder()` returning `Builder<Double>` could not feed a `Builder<@nullable Double>`. * the 16 remaining statics on `Pair`: `zip`, `toMap`, `forEach`, `forEachIndexed`, `adjacents`, `firstAnd` and the rest. Together these account for 62 of the findings, most of them in `RelMdSize`, which computes a size per column and has no size for some of them.
Same treatment `FlatLists` got. `java.util.List` declares `<T extends @nullable Object> T[] toArray(T[] a)`; the override declared `<T> @nullable T[] toArray(T @nullable [] a)` and then cast the argument back to non-null to read its length. `Object[] toArray()` carried a `ConsList<@nullable E> this` receiver parameter, which is how the Checker Framework said "only when E is nullable". The two delegated `toArray()` calls are cast, and say why: the JDK model reports `@Nullable Object[]` at a call site while the override check accepts only `Object[]` (uber/NullAway#1728).
…alues A profiled row carries a value per column, and a column may have none, so `Collector.add` and its three overrides take `List<@nullable Comparable>` and `CompositeCollector` keeps a `@Nullable Comparable[]`. `FlatLists.of(T, T, T)`, the six statics of `CompositeList` and `SqlBasicCall.set` take the nullable element bound of the lists they build. `LatticeSuggester` keys a node by its parent, and a root has none; `AggregateReduceFunctionsRule` names the extra columns it projects, of which the new ones have no name yet.
…ived its reason `SqlToRelConverter` reads the project it has just cast rather than casting it at each use, and asks for a `DmlNamespace` after `isWrapperFor` has established there is one. `SqlValidatorImpl` collects aliases into a list that admits the null a child of the FROM clause may have, and looks up a column by an index it took from the map it is reading. `JavaRowFormat.copy` returns a list of statements and never null, so the `castNonNull` around it in `EnumUtils` goes away. `FilterProjectTransposeRule` answers `replaceIfs` with null when the input has no distribution, rather than a singleton list holding null. `replaceIfs` takes a supplier that may answer null, and does the same thing with it. `CalciteCatalogReader` falls back on a family constant, which is what `firstNonNull` is for.
`ProfilerImpl` separates the two kinds of row it works with: a scanned row uses `NullSentinel` for a SQL null and so holds no Java null, while the sketch path builds a sparse row filled only at the ordinals of the space it feeds. `Collector.add` takes `List<? extends @nullable Comparable>` so it accepts both, and each collector casts the ordinals its own space owns. Type parameters that carry the nullable bound of what they build: `RexWindowBound.accept` and its override, `Functions.ignore2`, `Util.combine`, `SqlNodeList.toArray`, `HepPlanner.onCopyHook`, `EnumerableTableModify.keyOf` and the maps keyed by it, and `ArrayTable.asList`. `SqlNode.toList` and `RelBuilder` replace a method reference and a Guava call whose wildcard comes from bytecode with a lambda and a direct iterator check. `TableFunctionScanNode` drops its raw `Enumerable` for a typed one. `ArrayTable.permute` is suppressed: an array creation keeps a non-null component type whatever it is assigned to, so writing a nullable element reports even though both arrays are declared `@Nullable Comparable[]`.
…nterface declares A visitor over SqlNode declares itself SqlBasicVisitor<@nullable Void> or SqlVisitor<@nullable Void>, so its visit methods return @nullable Void. The overrides narrowed that to Void, which for a type whose only value is null promises nothing, and NullAway could not infer R for SqlNode.accept: the argument constrained it to be both @nonnull and @nullable. See uber/NullAway#1733
NullAway 0.12.13 and later ship a RequireExplicitNullMarking Error Prone check that fails a top-level class which is neither annotated nor covered by an annotated package or module. It is what the OnlyNullMarked setting needs, and it is stricter than the LintTest check it replaces: a class that sits in a package with no package-info.java is reported by name, whether or not the package holds a package-info.java at all.
The Checker Framework covered :server as well, so NullAway takes it over. The module needs no source changes: its four main classes live in org.apache.calcite.server, a package that :core already declares @NullMarked, and the generated DDL parser sits under a javacc directory, which the XepExcludedPaths setting skips the way AskipDefs used to.
The Druid adapter now declares its package @NullMarked, and NullAway verifies it. Most of the change is stating in the signatures what the bodies already did: a visitor over a Druid column returns a pair whose halves are both absent when the column cannot be pushed down, an extraction function carries no granularity and no locale, and the filters and plans that writeFieldIf skips take an absent value. Three places said something the code did not mean. Only the Checker Framework needed the preconditions on DruidTable.create and DruidType.getTypeFromMetric, whose callers are now the ones that check. DruidProjectRule named a field null for an expression that is not an input reference, but splitProjects puts nothing but input references there, so the branch was dead. And a rolled-up column with no parent node dereferenced that parent, where the Table contract has said it may be absent since the method was introduced. The Jackson result classes keep non-null fields under a NullAway.Init suppression, since Druid always populates them; the three that depend on the analysis types the query asked for are @nullable, which is what the reader of aggregators already assumed.
The file adapter now declares its package @NullMarked, and NullAway verifies it. A CSV cell, a table name in a model, and a field configuration are all absent-able, and the signatures now say so: field() reads a row whose cells may be absent, the row converters carry a nullable element type, and FileSchema falls back to the source path through Util.firstNonNull rather than through the @contract on Util.first, which NullAway cannot read. Two lazily populated fields were the reason for the remaining reports. FileReader.getTable wrote its result into tableElement and returned nothing, so no caller could see that the field was populated; it is now readTable, which returns the element it read. The bad-source-column check in FileRowConverter looked the heading up twice, once to validate and once to take the index, and now does both at once. A model that names no file for a CSV table, or no url for an HTML table, was already a NullPointerException deeper in; requireNonNull names the missing operand instead.
The model that the adapter's own test uses names neither bootstrap.servers nor topic.name, because a table that injects its own consumer needs neither, so both options are optional and KafkaTableOptions now says so. That reaches KafkaRowConverter.rowDataType, whose topic name is absent for such a table; neither implementation looks at it. The bootstrap servers are required on the path that builds a consumer, and requireNonNull there names the missing operand rather than letting the Kafka client report it.
vlsi
commented
Aug 25, 2026
| */ | ||
| public void request(QueryType queryType, String data, Sink sink, | ||
| List<String> fieldNames, List<ColumnMetaData.Rep> fieldTypes, | ||
| List<String> fieldNames, List<ColumnMetaData.@Nullable Rep> fieldTypes, |
Contributor
Author
There was a problem hiding this comment.
This might better be List<? extends ColumnMetaData.@Nullable Rep>, however, AFAIK it would change public API signature, so it would not be 100% backward compatible
SparkValues read the rowType field, which AbstractRelNode keeps as a lazily computed cache, where it meant the row type its constructor was given; getRowType() is the accessor that always has one. EnumerableToSparkConverter throws before it reaches its unfinished body, so the body is gone and the comment that describes what it would generate stays. RexToLixTranslator.translateCondition passes its correlates argument straight to setCorrelates, which has always accepted null, so the parameter says so now. That is what lets SparkCalc convert a program that has no correlates.
The call factory for Babel's CREATE TABLE takes the collection type out of a symbol literal, and SqlLiteral.symbolValue returns null for a literal that holds no symbol. The parser always writes one, so requireNonNull states that rather than leaving the constructor to find out.
A Redis schema with no password is the ordinary case, and it reaches the pool config through RedisConfig and RedisJedisManager, both of which say so now. RedisSchema validated its operands through isEmptyObject, whose result NullAway cannot connect back to the value, so the checks name the value they read and read it once. RedisTable had a RedisEnumerator field that nothing ever read. The anonymous Enumerable in RedisTable.scan is now a named inner class. NullAway checks an anonymous class's overrides against the erased supertype, dropping the @nullable on its type argument, so it reported the enumerator() override as a nullability mismatch; a named subclass with the same type argument is accepted.
The push-down rule builds a search string from whichever of the two projections and the two row types the match happened to have, so those parameters are optional and the signature says so. A literal that is neither numeric nor CHAR yields no search text, which is how getFilter already reads the result. SplunkResultEnumerator reads the CSV header in its constructor, and a header it could not read leaves the field names absent; moveNext now stops instead of dereferencing them. close() swallowed the NullPointerException it raised on a null Closeable, and returns early instead.
A MongoDB document has no value for a field it does not carry, so a projection of one field enumerates nulls, and the getter, the enumerator and the enumerable that carry it now say so. That needed the element type of AbstractEnumerable to admit null, which Enumerable and Queryable have admitted all along; the four interfaces between them said otherwise, and now agree. The filter translator builds its documents with JsonBuilder, whose maps and lists hold absent values, and it passes a null operator to mean equality, which translateOp2 has always read that way. The two anonymous Enumerables in MongoTable are named classes, to avoid uber/NullAway#1746.
A Cassandra row has no value for a column it never set, so the enumerator and the enumerable that carries it enumerate nulls. The tuple components a STRUCT holds are already collected through requireNonNull, which is where the comment saying null cannot appear inside a collection lives. The enumerable is a named class rather than an anonymous one, to avoid uber/NullAway#1746.
The Pig rel nodes look down the tree for the table they act on, and a tree with no table underneath returns none, which is what RelNode.getTable has always said. PigToEnumerableConverter read the rowType field, AbstractRelNode's lazily computed cache, where it meant this node's row type. A model that names no file or no columns for a Pig table reached the File and the array with a null; requireNonNull names the missing operand.
An Arrow vector holds a null wherever the column has no value, which getValue returns for a timestamp and the enumerator hands on, so the enumerator, the enumerable and the query that builds it carry a nullable element type. The precondition on query's field list is one the ImmutableIntList parameter already makes.
An InnoDB row has no value for a column that holds none, which the enumerator returns and the row array carries. The implementor and the internal expression node are filled in as the translation proceeds, so their fields are marked NullAway.Init rather than pretending a half-built object never exists. A model that names no sql file or no data file path reached the schema with a null; requireNonNull names the missing operand.
Piglet keeps four maps from a relation to its alias and its Pig operator, and looking a name up in them can miss, which the getters now say. Handler looked its relations up the same way and pushed whatever came back, including nothing; it now reports the unknown name instead of failing later in the builder. PigTable.scan returned null rather than an enumerable, and nothing could have used it; it throws. SqlUserDefinedFunction declared its operand type inference non-null though SqlFunction below it has always accepted none, which is what PigUserDefinedFunction passes.
…left behind The query enumerable still had the non-null element type, and its anonymous form ran into uber/NullAway#1746 once the type argument admitted null. It is a named class now, like the ones in :redis, :mongodb and :cassandra.
…e class allows Aggregate.copy is handed no grouping sets when the aggregate has a single group, and PigAggregate.copy passes that straight to its own constructor, which declared them required.
…autostyle wants Lint:skip
A Geode entry has no value for a field it does not carry, which the converters return and the enumerator hands on. The two schema factories read four operands out of the model and passed them on unchecked; requireNonNull names whichever one is missing. The lazily built table maps and the limit an implement context may not have say so. Region's value type is a bytecode wildcard whose upper bound reads as nullable, so the value constraint cannot be held in a Class<?>; a raw Class avoids it. See uber/NullAway#1732.
An Elasticsearch hit carries either _source or fields and never both, so each of the two is absent half the time, and a document has no value for a field it does not carry. That runs through the getters, the row converters and the aggregation buckets, whose key is absent for a missing bucket. The predicate analyzer reads a literal that may hold no value: a range bound needs one and says so, while a term query writes whatever it got. A LIKE with no escape, a projection that is not an item reference, and an expression the analyzer cannot convert are all absent results the callers already handled. The schema factory takes the credentials and the path prefix from the model, where they are optional.
An os table function returns a row whose columns are absent wherever the command printed nothing, which the enumerators and the line parsers now carry. Ten of them built the same anonymous enumerable over an osquery table; they share OsQueryEnumerable instead, which also avoids uber/NullAway#1746, as do the named enumerator in the stdin function and the named line parser in vmstat. os.name is absent on a JVM that does not publish it, and the table functions switch on it. SqlShell prints a column that has no value, and looks a column label up in a map that may not hold it. The Avatica server for Chinook holds its server and its meta instance from the point it starts them.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Preview, not ready to merge. NullAway still reports 576 errors in
calcite-coreand 126 incalcite-linq4j, so the nullness CI job is red on purpose. The point is to show the migration and the shape of what remains. See CALCITE-7736.Why
The Checker Framework needs a Gradle plugin of its own, 48
.astubfiles that patch the nullness of the JDK and of third-party libraries, and two dedicated CI jobs. NullAway is a single Error Prone check: no separate plugin, no stub files, and nullness models for the JDK and for popular libraries out of the box. The annotations come from JSpecify, a specification that several checkers read, rather than from one checker's own package.What
Six commits, each doing one thing:
@Nullableand@NonNullfrom the Checker Framework to JSpecify@NullMarkedon the packages that NullAway verifiesLintTest.testLintNullMarked@PolyNull,@MonotonicNonNull,@Pure, the initialization annotations, and the rest@PolyNullwith@ContractThe rename commit is worth skimming rather than reading. Commits 1 to 3 do not build on their own, because the source still carries Checker Framework annotations after
checker-qualis gone; from commit 4 onward every commit compiles.NullAway is configured in JSpecify mode with the experimental generics support (
JSpecifyExperimental,HandleWildcardGenerics,JSpecifyJDKModels,WarnOnGenericInferenceFailure) and withCheckContracts. It is an error in the projects listed innullawayProjectsand off elsewhere, so a nullness problem fails one CI job rather than every test job.org.apache.calcite.linq4j.annotationsis new and holds@Contract,@MonotonicNonNull,@RequiresNonNull,@EnsuresNonNulland@EnsuresNonNullIf. NullAway matches these by the last component of their name rather than by their package, so Calcite declares its own and takes no dependency on the checker.The part worth reviewing
The two tools default an unwritten type parameter bound in opposite directions. CLIMB-to-top gives implicit bounds the top qualifier, so
<T>under the Checker Framework means<T extends @Nullable Object>; JSpecify fills inObject, which under@NullMarkedis non-null. Every unbounded type parameter therefore changed meaning, and Calcite relied on the Checker Framework reading —SqlShuttle extends SqlBasicVisitor<@Nullable SqlNode>was passed toSqlNode.accept(SqlVisitor<R>)with no suppression, which typechecks only ifRadmits a nullable argument.Writing the bound out at 61 declarations took NullAway from 1126 errors to 576.
Pair.ofalone was worth 132: its class already had the bounds, but a static factory declares type parameters of its own.The erasure is unchanged, so these are binary compatible.
How to verify
Needs JDK 21, which Error Prone 2.43 and later require.
classes,testClasses,checkstyleMain,checkstyleTestandautostyleCheckpass.:core:testand:linq4j:testrun 18866 tests with no failures.Open questions
nullawayProjectslists:linq4jand:core. The Checker Framework jobs also covered:server.calcite-annotationsmodule rather than incalcite-linq4j?IndexOutOfBoundsExceptionwhen a@Contractclause names more arguments than the call site passes:ContractHandler.onDataflowVisitMethodInvocationreads arguments by the antecedent's length, and validates the arity on declarations but not at call sites. Worth reporting upstream. Avoided here by not annotating receiver parameters or varargs methods.