fix!: replace assert-based validation with real exceptions

[nielspardon]

assert compiles behind $assertionsDisabled, so the invariant checks in :core and :isthmus fired in Gradle's test JVMs (which enable -ea by default) and nowhere else. The invariants were documented by the code and enforced in CI, but not in the place where a malformed plan actually causes damage. And when they do run, AssertionError is an Error, not an Exception, so a host that wraps plan conversion in catch (Exception e) to report a bad plan does not catch it — it propagates as a hard failure.

Two of these checks cost a consumer something today:

• Expression.NestedList — getType() reads values().get(0), so an empty nested list throws IndexOutOfBoundsException from an unrelated place instead of the assert's "use ExpressionCreator.emptyList()" hint.
• SubstraitRelNodeConverter targetTable — a missing catalog entry passes null into LogicalTableModify.create, which NPEs inside Calcite, rather than producing the clear "Table not found in Calcite catalog" message the same file already produces 150 lines earlier.

The rest are invariants that today's call sites cannot violate. They stay as explicit guards — an assert that genuinely cannot fail costs nothing to write as a throw — but they are not fixes for anything.

The rule applied

• caller-facing invariant (builder input, a proto message, a Calcite RelNode) → IllegalArgumentException
• internal invariant / configuration ("cannot happen unless the converter is misconfigured") → IllegalStateException
• no bare assert left, anywhere.

:core

Site	Now throws
VirtualTableScan.check() — name count vs depth-first named-field count, row shape, rows not nullable, row field types match schema	IllegalArgumentException
Expression.NestedList.check() — non-empty, all values the same type	IllegalArgumentException
ExpressionProtoConverter.toLiteral() — guard; all call sites are statically Expression.Literal	IllegalArgumentException
VariadicParameterConsistencyValidator (already threw AssertionError unconditionally)	IllegalArgumentException

VirtualTableScan.check()'s compound assert is split into one check per invariant so the message names the mismatched counts, and the schema's field types are now derived once per relation instead of once per row.

NestedList's homogeneity check compares element types modulo nullability. The assert it replaces compared types exactly, which under -ea rejected SELECT ARRAY[not_null_col, nullable_col]: SQL list constructors do not cast their values to a common type, so such a list legitimately holds values that differ only in nullability. Enforcing the old comparison in production would have broken queries that convert today.

:isthmus

Site	Now throws
SubstraitRelNodeConverter ×2 — relBuilder.getRelOptSchema()	IllegalStateException
SubstraitRelNodeConverter — targetTable	IllegalStateException, reusing the existing message
SubstraitRelNodeConverter — catalogReader, asserted on a value just assigned from a cast	dropped as dead
SqlMapValueConstructorCallConverter — operand count even	IllegalArgumentException
CallConverters.CASE — operand count odd	IllegalArgumentException
CreateTable.copy(), CreateView.copy() — inputs.size() == 1	IllegalArgumentException
SubstraitRelVisitor ×2 (INSERT/DELETE, UPDATE) — guard; Calcite's TableModify constructor already dereferences the table	IllegalArgumentException
FunctionConverter.matchKeys() — guard; both lists come from the same operand stream	IllegalStateException

Two small refactors fall out of this: a requireTable(TableModify) helper in SubstraitRelVisitor (which also collapses the repeated modify.getTable() calls) and requireRelOptSchema() / requireCatalogReader() helpers in SubstraitRelNodeConverter, shared by the write and update paths and protected so subclasses overriding those visit methods can reuse them. The targetTable null check deliberately stays after the switch — the CTAS branch returns earlier and legitimately has no pre-existing table, so hoisting it to the lookup would break handleCreateTableAs.

SqlMapValueConstructorCallConverter also grows two fixes the operand-count check exposed: it ran after the cast that would already have thrown ClassCastException, and the map it builds is now insertion-ordered so a map literal keeps the key order written in the query.

The stale @throws AssertionError Javadoc tags are updated.

Guarding against regressions

A custom PMD rule AvoidAssertStatement (//AssertStatement) in substrait-pmd.xml fails the build on any new assert, and its violation message states the IllegalArgumentException / IllegalStateException rule above at the offending line. PMD scans test source sets too, so the one assert in isthmus test code (RepeatRel.copy()) is converted as well.

Two calls worth a second opinion

• No grace period. Plan.Root.check() is the precedent — hard IllegalArgumentException for the invariant, LOGGER.warn only for its one legacy allowance.
• VirtualTableScan row/schema types still compare with exact Type.equals, so nullability must match precisely there. That is the strictest reading of the spec and the most likely thing to reject another producer's plan, but relaxing it would be a semantic change rather than part of this one.

:spark needed no changes: its Scala sources use require(...), not the Java assert keyword.

BREAKING CHANGE: validation that previously used assert now throws unconditionally. Callers catching AssertionError must catch IllegalArgumentException or IllegalStateException instead, and code running without -ea — i.e. most deployments — will now see these checks fire: VirtualTableScan and Expression.NestedList reject malformed input both from their builders and from ProtoRelConverter / ProtoExpressionConverter, and VariadicParameterConsistencyValidator throws IllegalArgumentException rather than AssertionError.

Closes #1047

🤖 Generated with AI

[andrew-coleman]

One blocker: the NestedList check now rejects SELECT ARRAY[nullable_col, notnull_col], which converts on main. Comments inline.

Three of the new checks are unreachable (VirtualTableScan null elements, toLiteral, requireTable) — the PR description cites them as motivating bugs.

Untested on the isthmus side: every new throw except requireCatalogReader.

[andrew-coleman]

Pre-existing, not this PR — flagging for a separate issue. Line 715 converts the input, then case CTAS re-converts it in handleCreateTableAs. With a rel_anchor on the input that throws UnsupportedOperationException: Duplicate rel_anchor=1. Base behaves identically.

[andrew-coleman]

Please add AvoidAssertStatement to the rule list in AGENTS.md line 126-128.

[andrew-coleman]

RelOptTableImpl.create(RelOptSchema, RelDataType, List<String>, Expression) returns the schema you pass from getRelOptSchema() — replaces this class. Also extend()/toRel() currently forward to the undecorated delegate.