isthmus: a NamedScan's declared schema is not reconciled with the catalog it resolves against

[alexandrefimov]

visit(NamedScan) builds the relation from the name alone:

RelNode node = relBuilder.scan(namedScan.getNames()).build();

When no catalog is supplied, SchemaCollector builds one out of the plan and reads getInitialSchema() for it; the comparison it makes there is between two reads of the same table within the plan, not between the plan and a catalog. When a catalog is supplied that path is not taken, and the catalog-backed conversion does not compare the catalog row type with the declared schema.

The spec makes the declared schema — base_schema in the proto — required, and gives it a job: "Direct Schema — Defines the schema of the output of the read (before any projection or emit remapping/hiding)". What it does not say is what a consumer should do when it resolves the name against a catalog whose schema differs.

Today it does nothing, quietly. Reproduced with Isthmus 0.103.0 using Impala's catalog reader, one plan throughout, declaring t_rn(c0 i64 not null, c1 i64) and naming both columns in Plan.Root.names:

{"relations":[{"root":{"input":{"read":{"baseSchema":{"names":["c0","c1"],
"struct":{"types":[{"i64":{"nullability":"NULLABILITY_REQUIRED"}},
{"i64":{"nullability":"NULLABILITY_NULLABLE"}}],"nullability":"NULLABILITY_REQUIRED"}},
"namedTable":{"names":["t_rn"]}}},"names":["c0","c1"]}}],
"version":{"minorNumber":102,"producer":"case-corpus"}}

The catalog is Impala's own, holding one table per row of the table below:

// t_rn is created in the catalog, then Impala's frontend is asked to analyse a
// statement naming it, which is where its CalciteCatalogReader comes from.
addTestTable("create table " + db + ".t_rn (c0 bigint, c1 bigint) stored as parquet");
CalciteCatalogReader reader = analyse("select 1 from t_rn", db).getCatalogReader();

// The provider's type factory is Impala's; getRelBuilder is overridden to build on
// Impala's RelOptCluster, so the relations are the ones its planner expects.
RelRoot root = new SubstraitToCalcite(provider, reader)
    .convert(new ProtoPlanConverter().from(protoPlan).getRoots().get(0));
root.rel.getRowType();

visit(NamedScan) is unchanged on main at ee9f3d2, and this plan carries no projection, so #1280 does not touch it.

catalog	root.rel.getRowType()
(c0 bigint, c1 bigint)	[c0 BIGINT, c1 BIGINT] — the catalog's nullable c0 is accepted despite the plan declaring it required
(c0 string, c1 bigint)	[c0 VARCHAR, c1 BIGINT]
(c0 bigint)	[c0 BIGINT]
(x0 bigint, x1 bigint)	[x0 BIGINT, x1 BIGINT]; root.validatedRowType and root.fields carry c0, c1, applied by position
(c0 bigint, c1 bigint, c2 bigint)	IndexOutOfBoundsException: Index 2 out of bounds for length 2, from renameFields

Only the last row says anything, and what it says is an index error out of the root renaming rather than a disagreement about a schema.

A consumer can compare the two itself, and the one I am measuring with does. But whether Isthmus should — reconcile, refuse, or hand the caller both and let it decide — seems worth settling once rather than in each consumer.

Related but distinct: #1204 (a read's filter and projection are dropped) and #1178 (the two renameFields walkers).

[alexandrefimov]

A second way the disagreement shows up, and the condition on seeing it at all.

ExpressionRexConverter.visit(FieldReference) types the reference from the plan:

rexInputRef =
    new RexInputRef(field.offset(), typeConverter.toCalcite(typeFactory, expr.getType()));

The scan beneath it is typed from the catalog, so the two can disagree. Nothing in the conversion compares them. What catches it, when anything does, is Calcite: the Project constructor runs assert isValid(Litmus.THROW, null), and with JVM assertions enabled that throws

AssertionError: type mismatch: ref: DECIMAL(10, 2) NOT NULL input: DECIMAL(10, 2)

With assertions off the same plans convert and go on to plan. I ran the sweep both ways: with -ea, seven cases stop here; with -da, all seven convert, and the relation carries a field reference whose type disagrees with its input. So this is not a second failure so much as the first one again — silent unless a build happens to have assertions on.

Reproducer for the message above (decimal_add): a read of t_dec whose declared schema makes its decimal columns required, with an add over two of them, against create table t_dec (c0 DECIMAL(10,2), c1 DECIMAL(5,1), c2 DECIMAL(38,10), c3 DECIMAL(38,10)) stored as parquet — nullable, because Impala's HDFS columns always are. Isthmus 0.103.0, Calcite 1.42.0, JDK 17.0.19; the converter is unchanged on main at ee9f3d2.

Counts, from that one run and a corpus I cannot point at a public revision of: of 56 plans reading a named table, 52 declare at least one column required, so against an always-nullable catalog the disagreement is present in almost all of them. It becomes visible in seven: decimal_add, decimal_add_overflow, decimal_divide, decimal_multiply, decimal_multiply_overflow (all arithmetic over a declared-required decimal), emit_filter (a boolean condition) and emit_project, where the reference is VARCHAR NOT NULL and the input VARCHAR(2147483647). A read that never refers to such a column converts either way.

So, more precisely than the question I left above: how should Isthmus handle this mismatch before constructing field references whose types disagree with their input?