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.

Three concrete costs today, which is what a consumer sees:

• ExpressionProtoConverter.toLiteral() — a protobuf oneof getter returns the default
  instance when its case is not set, so a non-literal where a literal is required is silently
  converted to an empty literal. We emit a wrong plan instead of failing.
• 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 becomes an NPE inside
  Calcite rather than the clear "Table not found in Calcite catalog" message the same file
  already produces 150 lines earlier.

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. An assert that can fail is worse than useless; an assert
  that genuinely cannot fail costs nothing to write as an explicit throw.

:core

Site	Now throws
VirtualTableScan.check() — name count vs depth-first named-field count, no null names/rows, row shape, rows not nullable, row field types match schema	IllegalArgumentException
Expression.NestedList.check() — non-empty, all values same type	IllegalArgumentException
ExpressionProtoConverter.toLiteral()	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 null-element checks now run before the
row.nullable() loop — a null row previously NPE'd there before reaching its own assert.

:isthmus

Site	Now throws
SubstraitRelVisitor ×2 (INSERT/DELETE, UPDATE) — modify.getTable()	IllegalArgumentException
SubstraitRelNodeConverter ×2 — relBuilder.getRelOptSchema()	IllegalStateException
SubstraitRelNodeConverter — catalogReader, asserted on a value just assigned from a cast	dropped as dead
SubstraitRelNodeConverter — targetTable	IllegalStateException, reusing the existing message
SqlMapValueConstructorCallConverter — operand count even	IllegalArgumentException
CallConverters.CASE — operand count odd	IllegalArgumentException
FunctionConverter.matchKeys() — private, internal	IllegalStateException
CreateTable.copy(), CreateView.copy() — inputs.size() == 1	IllegalArgumentException

Two small refactors fall out of this: a requireTable(TableModify) helper in
SubstraitRelVisitor (which also collapses the repeated modify.getTable() calls) and a
requireRelOptSchema() helper in SubstraitRelNodeConverter shared by both schema checks. 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.

The three 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.
• Row/schema types still compare with exact Type.equals, so nullability must match
  precisely. 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

[alexandrefimov]

I don’t think this check is quite enough yet. It verifies that the RelBuilder has a RelOptSchema, but both callers ultimately need a Prepare.CatalogReader.

In the UPDATE path, the reader comes from table.getRelOptSchema(), which Calcite marks @Nullable. After dropping the old null assertion, we can still build a LogicalTableModify with a null catalog reader and get an NPE later from getExpectedInputRowType(). In the write path, a non-CatalogReader implementation will fail at the cast with ClassCastException, rather than the IllegalStateException this PR is aiming for.

Could we validate the actual value as a Prepare.CatalogReader before creating LogicalTableModify, and add a small test for the null and wrong-type cases?

[nielspardon]

You're right on both counts, and the null one is worse than I assumed — thanks for catching it.

I dropped the catalogReader != null assert because the issue's table listed it as a dead check on "a value just assigned from a cast". That was wrong: the value comes from table.getRelOptSchema(), which Calcite declares @Nullable, and (Prepare.CatalogReader) null casts happily. It's also a different schema from the relBuilder.getRelOptSchema() that requireRelOptSchema() validates, so that helper never covered it. TableModify then stores the reader with no null check (the field is non-@Nullable, unlike the ones right below it) and only dereferences it in getExpectedInputRowType(), exactly as you describe.

Fixed in 45780f2 with a requireCatalogReader helper used at both sites. It narrows via instanceof, so a single check covers null and wrong-type, and the message names the class it actually got. The schema sources are unchanged — UPDATE still reads it from the resolved table, the write path from the RelBuilder — so this is validation only.

Tests are in CatalogReaderValidationTest: a RelOptSchema that resolves tables through a real catalog but deliberately isn't a CatalogReader, handing out tables whose getRelOptSchema() is either that schema or null. Three cases: write path wrong-type, update path null, update path wrong-type. I checked they fail without the fix, and the null case is the interesting one:

namedUpdateRejectsTableSchemaThatIsNotACatalogReader() FAILED
    expected: <IllegalStateException> but was: <ClassCastException>
namedWriteRejectsSchemaThatIsNotACatalogReader() FAILED
    expected: <IllegalStateException> but was: <ClassCastException>
namedUpdateRejectsTableWithoutASchema() FAILED
    Expected java.lang.IllegalStateException to be thrown, but nothing was thrown.

That last line is the point: conversion completed and produced a LogicalTableModify carrying a null catalog reader, with the failure deferred to whoever touched it next.

[alexandrefimov]

Thanks for the detailed follow-up! The fix and tests look good to me.