[CALCITE-7736] Replace the Checker Framework with NullAway and JSpecify - #5213
Draft
vlsi wants to merge 69 commits into
Draft
[CALCITE-7736] Replace the Checker Framework with NullAway and JSpecify#5213vlsi wants to merge 69 commits into
vlsi wants to merge 69 commits into
Conversation
This was referenced Aug 24, 2026
Closed
Closed
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
vlsi
force-pushed
the
CALCITE-7736
branch
4 times, most recently
from
August 26, 2026 09:33
ca7e071 to
3a5b15e
Compare
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -403,6 +411,11 @@ | |||
| actualMessage = Util.toLinux(actualMessage); | |||
| } | |||
|
|
|||
| if (expectedMsgPattern == null) { | |||
| actualException.printStackTrace(); | |||
vlsi
force-pushed
the
CALCITE-7736
branch
2 times, most recently
from
August 26, 2026 10:54
63d4152 to
5df3a4d
Compare
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… 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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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)`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`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[]`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…autostyle wants Lint:skip Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…verification A model that names no directory for a CSV schema, and no file for a CSV table, reached the File with a null; requireNonNull names the missing operand. The filterable table pushed down a literal that may hold no value. The maze enumerates without a solution set when the table is asked for the maze alone, which is what the null argument means. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The test fixtures carry absent values everywhere a SQL value can be null: the expected result of checkScalar and checkString, the column a result set reads, the origins and unique keys the metadata query may not know, and the alias a Pig relation may not have. The mock catalog and the mock planner fill their fields in as registration proceeds, so those are marked NullAway.Init. Twelve classes took the @nullable argument that Object.equals has always allowed. DiffRepository reads a DOM, where a node list yields no node past its length and an attribute may carry no value; the reads that cannot miss say so by name. The eight schemata packages that had no package-info.java now have one. BaseQueryable in :linq4j required a provider, though Smalls builds one that overrides enumerator() and never asks a provider to execute it; getProvider still refuses to return null. Four anonymous enumerables became named classes, to avoid uber/NullAway#1746. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The benchmarks keep their sources in the jmh source set rather than a main one, so the verification follows them there, and the errorprone CI job builds jmhClasses alongside classes. A JMH state class is filled in by the @setup methods and the @PARAM values, which is what NullAway.Init says; the rest is the usual: a statistics map that a phase may not have produced, an employee with no commission, and an edge the graph may not hold. Two Error Prone warnings that the jmh source set had never been built against are fixed rather than suppressed: a helper that reads no instance state is static, and a parse failure during setup is thrown rather than printed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Codex GPT 5.6-Terra <codex@openai.com>
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
2 participants
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.