From ab339fe4b6af9d57e323bec29da2489e2a540601 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Wed, 26 Aug 2026 14:55:21 +0300 Subject: [PATCH 1/6] fix(isthmus)!: round-trip a virtual table whose rows are not all literals Calcite has no relation that carries expressions the way a Substrait virtual table does, so the conversion expanded one into a UNION ALL of a single-row projection per row. Nothing in that shape says it was a table: converting the plan back gave the projection, and the relation changed under a round trip. Emit an isthmus-owned relation instead -- VirtualTable, holding the rows as RexNodes and the schema as its row type -- and recognise it by type on the way back, the way CreateTable and CreateView already are. Recognition by type cannot over-match a union someone wrote out of the same parts, and no planner rule can erase it: UNION_REMOVE strips the one-input union a single-row table expanded to, and UNION_MERGE rewrites into the same shape. Carrying the schema on the relation is also what reaches a struct column, whose names cannot be paired with a row type after the fact. A consumer whose planner only knows Calcite's own relations can expand the table with VirtualTableExpansionRule. That is opt-in, isthmus never runs it, and it is one-way: the expansion converts back as the projection it is. BREAKING CHANGE: a virtual table whose rows are not all literals now converts to io.substrait.isthmus.calcite.rel.VirtualTable instead of a UNION ALL of single-row projections over an empty table. Add VirtualTableExpansionRule to a planner to get that shape back. --- .../isthmus/OuterReferenceResolver.java | 7 + .../isthmus/SubstraitRelNodeConverter.java | 54 ++-- .../isthmus/SubstraitRelVisitor.java | 24 ++ .../isthmus/calcite/rel/VirtualTable.java | 140 +++++++++++ .../rel/rules/VirtualTableExpansionRule.java | 137 ++++++++++ .../LiteralNullabilityRoundtripTest.java | 10 +- .../isthmus/OuterReferenceResolverTest.java | 38 +++ .../isthmus/VirtualTableScanTest.java | 62 ++--- .../substrait/isthmus/VirtualTableTest.java | 235 ++++++++++++++++++ 9 files changed, 615 insertions(+), 92 deletions(-) create mode 100644 isthmus/src/main/java/io/substrait/isthmus/calcite/rel/VirtualTable.java create mode 100644 isthmus/src/main/java/io/substrait/isthmus/calcite/rel/rules/VirtualTableExpansionRule.java create mode 100644 isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java diff --git a/isthmus/src/main/java/io/substrait/isthmus/OuterReferenceResolver.java b/isthmus/src/main/java/io/substrait/isthmus/OuterReferenceResolver.java index 6b6ce6da8..877c2bbe6 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/OuterReferenceResolver.java +++ b/isthmus/src/main/java/io/substrait/isthmus/OuterReferenceResolver.java @@ -1,5 +1,6 @@ package io.substrait.isthmus; +import io.substrait.isthmus.calcite.rel.VirtualTable; import java.util.HashMap; import java.util.IdentityHashMap; import java.util.Map; @@ -115,6 +116,12 @@ public RelNode visit(Correlate correlate) throws RuntimeException { @Override public RelNode visitOther(RelNode other) throws RuntimeException { + if (other instanceof VirtualTable) { + // A virtual table's rows are expressions, and a subquery among them binds outer references + // like one anywhere else. A subquery's relation is not an input, so walking inputs never + // reaches it; Filter and Project scan their own before they get here. + other.accept(rexVisitor); + } for (RelNode child : other.getInputs()) { reverseAccept(child); } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java index 0138300ad..d139dc63e 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java @@ -11,6 +11,7 @@ import io.substrait.hint.Hint; import io.substrait.isthmus.calcite.rel.CreateTable; import io.substrait.isthmus.calcite.rel.CreateView; +import io.substrait.isthmus.calcite.rel.VirtualTable; import io.substrait.isthmus.expression.AggregateFunctionConverter; import io.substrait.isthmus.expression.ExpressionRexConverter; import io.substrait.isthmus.expression.ScalarFunctionConverter; @@ -62,6 +63,7 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.Stream; +import org.apache.calcite.plan.Convention; import org.apache.calcite.plan.RelOptSchema; import org.apache.calcite.plan.RelOptTable; import org.apache.calcite.plan.RelOptUtil; @@ -77,7 +79,6 @@ import org.apache.calcite.rel.core.TableModify; import org.apache.calcite.rel.logical.LogicalProject; import org.apache.calcite.rel.logical.LogicalTableModify; -import org.apache.calcite.rel.logical.LogicalUnion; import org.apache.calcite.rel.logical.LogicalValues; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rel.type.RelDataTypeFactory; @@ -869,45 +870,18 @@ public RelNode visit(VirtualTableScan virtualTableScan, Context context) { LogicalValues.create(relBuilder.getCluster(), rowType, tuplesBuilder.build()), virtualTableScan); } else { - // A row that does not fit a LogicalValues tuple is computed instead: we create a - // LogicalProject for each row to compute its values, and combine them together using a - // LogicalUnion. For example the following: - // - // VirtualTable - // (e1, e2) - // (e3, e4) - // - // Becomes: - // - // LogicalUnion(all=[true]) - // LogicalProject(exprs=[e1, e2]) - // - // LogicalProject(exprs=[e3, e4]) - // - // - - RelDataType emptyRowType = typeFactory.createStructType(List.of(), List.of()); - ImmutableList> emptyRowValue = ImmutableList.of(ImmutableList.of()); - - List projects = new ArrayList<>(); - for (final List rexRow : convertedRows) { - RelNode values = LogicalValues.create(relBuilder.getCluster(), emptyRowType, emptyRowValue); - RelNode project = - LogicalProject.create( - values, Collections.emptyList(), rexRow, rowType, Collections.emptySet()); - projects.add(project); - } - RelNode union = LogicalUnion.create(projects, true); - - // Apply a final LogicalProject on top to capture the field names from the VirtualTable - List topProjectExprs = new ArrayList<>(); - for (int i = 0; i < rowType.getFieldCount(); i++) { - topProjectExprs.add(rexBuilder.makeInputRef(union, i)); - } - RelNode topProject = - LogicalProject.create( - union, Collections.emptyList(), topProjectExprs, rowType, Collections.emptySet()); - return applyRelCommon(topProject, virtualTableScan, topProject); + // A row that does not fit a LogicalValues tuple keeps its expressions, in a relation of our + // own: Calcite has none that holds them, and expanding the table into a projection per row + // does not come back -- the projection is what converts back, and the table is gone. A + // consumer whose planner only knows Calcite's own relations can expand it with + // VirtualTableExpansionRule. + return applyRelCommon( + new VirtualTable( + relBuilder.getCluster(), + relBuilder.getCluster().traitSetOf(Convention.NONE), + rowType, + convertedRows), + virtualTableScan); } } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java index 8a434e998..1743fa18a 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java @@ -8,6 +8,7 @@ import io.substrait.extension.SimpleExtension; import io.substrait.isthmus.calcite.rel.CreateTable; import io.substrait.isthmus.calcite.rel.CreateView; +import io.substrait.isthmus.calcite.rel.VirtualTable; import io.substrait.isthmus.expression.AggregateFunctionConverter; import io.substrait.isthmus.expression.LiteralConverter; import io.substrait.isthmus.expression.RexExpressionConverter; @@ -932,6 +933,26 @@ public Rel handleCreateView(CreateView createView) { .build(); } + /** + * Handles the isthmus {@link VirtualTable}, which is what a virtual table whose rows are not all + * literals converts to. + * + * @param virtualTable Calcite virtual table + * @return Substrait virtual table scan + */ + public Rel handleVirtualTable(VirtualTable virtualTable) { + List rows = new ArrayList<>(virtualTable.getRows().size()); + for (List row : virtualTable.getRows()) { + List fields = + row.stream().map(this::toExpression).collect(Collectors.toUnmodifiableList()); + rows.add(ExpressionCreator.nestedStruct(false, fields)); + } + return VirtualTableScan.builder() + .initialSchema(typeConverter.toNamedStruct(virtualTable.getRowType())) + .addAllRows(rows) + .build(); + } + /** * Visits other Calcite nodes (e.g., DDL wrappers). * @@ -946,6 +967,9 @@ public Rel visitOther(RelNode other) { } else if (other instanceof CreateView) { return handleCreateView((CreateView) other); + + } else if (other instanceof VirtualTable) { + return handleVirtualTable((VirtualTable) other); } throw new UnsupportedOperationException("Unable to handle node: " + other); } diff --git a/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/VirtualTable.java b/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/VirtualTable.java new file mode 100644 index 000000000..8df513991 --- /dev/null +++ b/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/VirtualTable.java @@ -0,0 +1,140 @@ +package io.substrait.isthmus.calcite.rel; + +import com.google.common.collect.ImmutableList; +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Collectors; +import org.apache.calcite.plan.RelOptCluster; +import org.apache.calcite.plan.RelTraitSet; +import org.apache.calcite.rel.AbstractRelNode; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.RelWriter; +import org.apache.calcite.rel.metadata.RelMetadataQuery; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.rex.RexShuttle; +import org.apache.calcite.sql.SqlExplainLevel; + +/** + * A table of rows given as expressions, which is what a Substrait virtual table is and what Calcite + * has no relation for: {@link org.apache.calcite.rel.core.Values} holds literals and nothing else. + * + *

Isthmus emits this for a virtual table whose rows do not all fit Values tuples, and recognises + * it by type on the way back, so the relation comes back as the one it went in as. Expanding the + * rows into stock Calcite relations -- a projection per row, unioned -- loses that: the shape a + * planner leaves behind is a projection over an empty table, which is what converts back. A + * consumer that needs the expansion can ask for it with {@link + * io.substrait.isthmus.calcite.rel.rules.VirtualTableExpansionRule}, knowing it is one-way; nothing + * in isthmus runs that rule. + * + *

The rows are held at the row type's own field types, names included, so that the expansion has + * nothing left to derive. + */ +public class VirtualTable extends AbstractRelNode { + + private final ImmutableList> rows; + + /** + * VirtualTable constructor. + * + * @param cluster the cluster this relation belongs to + * @param traitSet the relation's traits + * @param rowType the table's row type, carrying the schema's field names + * @param rows one list of values per row, each value at the type its column is declared at + */ + public VirtualTable( + RelOptCluster cluster, + RelTraitSet traitSet, + RelDataType rowType, + List> rows) { + super(cluster, traitSet); + this.rowType = rowType; + ImmutableList.Builder> builder = ImmutableList.builder(); + for (List row : rows) { + builder.add(ImmutableList.copyOf(row)); + } + this.rows = builder.build(); + } + + /** + * Returns the table's rows. + * + * @return one list of values per row + */ + public List> getRows() { + return rows; + } + + @Override + protected RelDataType deriveRowType() { + return rowType; + } + + @Override + public double estimateRowCount(RelMetadataQuery mq) { + return rows.size(); + } + + /** + * Applies an expression rewrite to the rows. + * + *

Calcite rewrites a relation's expressions by handing it a shuttle, and a relation that does + * not pass one on keeps its expressions out of everything built on that -- finding the subqueries + * that bind outer references among them. + * + * @param shuttle the rewrite to apply + * @return this table with the rewritten rows, or itself where nothing changed + */ + @Override + public RelNode accept(RexShuttle shuttle) { + boolean changed = false; + List> rewritten = new ArrayList<>(rows.size()); + for (List row : rows) { + List rewrittenRow = new ArrayList<>(row.size()); + for (RexNode value : row) { + RexNode visited = value.accept(shuttle); + changed |= visited != value; + rewrittenRow.add(visited); + } + rewritten.add(rewrittenRow); + } + return changed ? new VirtualTable(getCluster(), getTraitSet(), rowType, rewritten) : this; + } + + /** + * Explains the node terms for plan output. + * + * @param pw plan writer + * @return the plan writer with this node's fields added + */ + @Override + public RelWriter explainTerms(RelWriter pw) { + return super.explainTerms(pw) + .itemIf("type", rowType, pw.getDetailLevel() == SqlExplainLevel.DIGEST_ATTRIBUTES) + .item( + "rows", + rows.stream() + .map( + row -> + row.stream() + .map(RexNode::toString) + .collect(Collectors.joining(", ", "{ ", " }"))) + .collect(Collectors.joining(", ", "[", "]"))); + } + + /** + * Copies this node with the given traits. + * + * @param traitSet the RelTraitSet + * @param inputs List of RelNodes, which has to be empty + * @return a copy of this node + * @throws IllegalArgumentException if given any input + */ + @Override + public RelNode copy(RelTraitSet traitSet, List inputs) { + if (!inputs.isEmpty()) { + throw new IllegalArgumentException("VirtualTable takes no inputs, but got " + inputs.size()); + } + return new VirtualTable(getCluster(), traitSet, rowType, rows); + } +} diff --git a/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/rules/VirtualTableExpansionRule.java b/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/rules/VirtualTableExpansionRule.java new file mode 100644 index 000000000..514630ed5 --- /dev/null +++ b/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/rules/VirtualTableExpansionRule.java @@ -0,0 +1,137 @@ +package io.substrait.isthmus.calcite.rel.rules; + +import com.google.common.collect.ImmutableList; +import io.substrait.isthmus.calcite.rel.VirtualTable; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import org.apache.calcite.plan.RelOptCluster; +import org.apache.calcite.plan.RelOptRuleCall; +import org.apache.calcite.plan.RelRule; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.core.RelFactories; +import org.apache.calcite.rel.logical.LogicalProject; +import org.apache.calcite.rel.logical.LogicalUnion; +import org.apache.calcite.rel.logical.LogicalValues; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rex.RexLiteral; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.tools.RelBuilderFactory; + +/** + * Expands a {@link VirtualTable} into stock Calcite relations: a projection computing each row over + * a single empty row, and a UNION ALL of those where there is more than one. + * + * 

{@code
+ * VirtualTable(rows=[{ e1, e2 }, { e3, e4 }])
+ *
+ *   LogicalUnion(all=[true])
+ *     LogicalProject(exprs=[e1, e2])
+ *       LogicalValues(tuples=[[{ }]])
+ *     LogicalProject(exprs=[e3, e4])
+ *       LogicalValues(tuples=[[{ }]])
+ * }
+ * + *

This is for a consumer whose planner only knows Calcite's own relations. It is one-way: the + * expansion is a plan like any other, and converting it back to Substrait gives the relation it is, + * not the virtual table it came from. Isthmus never runs it -- a plan it converts keeps the {@link + * VirtualTable}, which is what makes the round trip exact. + */ +public class VirtualTableExpansionRule extends RelRule { + + /** The rule instance to add to a planner. */ + public static final VirtualTableExpansionRule INSTANCE = Config.DEFAULT.toRule(); + + private VirtualTableExpansionRule(Config config) { + super(config); + } + + @Override + public void onMatch(RelOptRuleCall call) { + call.transformTo(expand(call.rel(0))); + } + + private static RelNode expand(VirtualTable virtualTable) { + RelOptCluster cluster = virtualTable.getCluster(); + RelDataType rowType = virtualTable.getRowType(); + if (virtualTable.getRows().isEmpty()) { + return LogicalValues.create(cluster, rowType, ImmutableList.of()); + } + + RelDataType emptyRowType = cluster.getTypeFactory().createStructType(List.of(), List.of()); + ImmutableList> singleEmptyRow = ImmutableList.of(ImmutableList.of()); + + List rowProjects = new ArrayList<>(); + for (List row : virtualTable.getRows()) { + RelNode emptyRow = LogicalValues.create(cluster, emptyRowType, singleEmptyRow); + rowProjects.add( + LogicalProject.create( + emptyRow, Collections.emptyList(), row, rowType, Collections.emptySet())); + } + // A one-input union is not a relation a planner keeps -- UNION_REMOVE strips it -- and the + // projection is what the expansion means anyway. + return rowProjects.size() == 1 ? rowProjects.get(0) : LogicalUnion.create(rowProjects, true); + } + + /** + * Rule configuration. + * + *

Written out rather than generated: the rule matches one relation and has nothing to + * configure, so the three properties {@link RelRule.Config} declares are all there is. + */ + public static class Config implements RelRule.Config { + + private static final OperandTransform TABLE = + operand -> operand.operand(VirtualTable.class).noInputs(); + + /** The configuration {@link VirtualTableExpansionRule#INSTANCE} is built from. */ + public static final Config DEFAULT = + new Config(RelFactories.LOGICAL_BUILDER, "VirtualTableExpansionRule", TABLE); + + private final RelBuilderFactory relBuilderFactory; + private final String description; + private final OperandTransform operandSupplier; + + private Config( + RelBuilderFactory relBuilderFactory, String description, OperandTransform operandSupplier) { + this.relBuilderFactory = relBuilderFactory; + this.description = description; + this.operandSupplier = operandSupplier; + } + + @Override + public VirtualTableExpansionRule toRule() { + return new VirtualTableExpansionRule(this); + } + + @Override + public RelBuilderFactory relBuilderFactory() { + return relBuilderFactory; + } + + @Override + public Config withRelBuilderFactory(RelBuilderFactory factory) { + return new Config(factory, description, operandSupplier); + } + + @Override + public String description() { + return description; + } + + @Override + public Config withDescription(String description) { + return new Config(relBuilderFactory, description, operandSupplier); + } + + @Override + public OperandTransform operandSupplier() { + return operandSupplier; + } + + @Override + public Config withOperandSupplier(OperandTransform transform) { + return new Config(relBuilderFactory, description, transform); + } + } +} diff --git a/isthmus/src/test/java/io/substrait/isthmus/LiteralNullabilityRoundtripTest.java b/isthmus/src/test/java/io/substrait/isthmus/LiteralNullabilityRoundtripTest.java index e19549264..5ccf12452 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/LiteralNullabilityRoundtripTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/LiteralNullabilityRoundtripTest.java @@ -132,10 +132,7 @@ void tupleUnwrapLeavesATruncatingCastAlone() { .build(); assertEquals( - "LogicalProject(A=[$0])\n" - + " LogicalUnion(all=[true])\n" - + " LogicalProject(A=[CAST('abcdef'):VARCHAR(3) NOT NULL])\n" - + " LogicalValues(tuples=[[{ }]])\n", + "VirtualTable(rows=[[{ CAST('abcdef'):VARCHAR(3) NOT NULL }]])\n", RelOptUtil.toString(substraitToCalcite.convert(overlong))); } @@ -160,10 +157,7 @@ void aDeclaredNullabilityCastKeepsTheRowOffTheTuplePath() { .build(); assertEquals( - "LogicalProject(A=[$0])\n" - + " LogicalUnion(all=[true])\n" - + " LogicalProject(A=[CAST(5):INTEGER])\n" - + " LogicalValues(tuples=[[{ }]])\n", + "VirtualTable(rows=[[{ CAST(5):INTEGER }]])\n", RelOptUtil.toString(substraitToCalcite.convert(castRow))); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/OuterReferenceResolverTest.java b/isthmus/src/test/java/io/substrait/isthmus/OuterReferenceResolverTest.java index a01d266cf..c24cd7419 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/OuterReferenceResolverTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/OuterReferenceResolverTest.java @@ -1,9 +1,13 @@ package io.substrait.isthmus; +import io.substrait.isthmus.calcite.rel.VirtualTable; +import java.util.List; +import org.apache.calcite.plan.Convention; import org.apache.calcite.rel.RelNode; import org.apache.calcite.rel.core.JoinRelType; import org.apache.calcite.rex.RexCorrelVariable; import org.apache.calcite.rex.RexNode; +import org.apache.calcite.rex.RexSubQuery; import org.apache.calcite.sql.parser.SqlParseException; import org.apache.calcite.tools.RelBuilder; import org.apache.calcite.util.Holder; @@ -129,6 +133,40 @@ void nestedApplyJoinQuery() throws SqlParseException { Assertions.assertNotEquals(anchor0, anchor1); } + /** + * A virtual table's rows are expressions, and a subquery among them is a tree the resolver only + * reaches through the row: a subquery's relation is not an input, so walking inputs never gets + * there and a correlation declared inside it is left unbound. + */ + @Test + void correlationInsideASubqueryInAVirtualTableRow() { + final Holder cor0 = Holder.empty(); + final RelNode correlated = + tpcDsRelBuilder + .scan("tpcds", "STORE_SALES") + .variable(cor0::set) + .scan("tpcds", "ITEM") + .filter( + tpcDsRelBuilder.equals( + tpcDsRelBuilder.field("I_ITEM_SK"), + tpcDsRelBuilder.field(cor0.get(), "SS_ITEM_SK"))) + .project(tpcDsRelBuilder.field("I_ITEM_SK")) + .correlate(JoinRelType.INNER, cor0.get().id, tpcDsRelBuilder.field(2, 0, "SS_ITEM_SK")) + .project(tpcDsRelBuilder.field("SS_ITEM_SK")) + .build(); + final RexNode subQuery = RexSubQuery.scalar(correlated); + + final RelNode virtualTable = + new VirtualTable( + tpcDsRelBuilder.getCluster(), + tpcDsRelBuilder.getCluster().traitSetOf(Convention.NONE), + tpcDsRelBuilder.getTypeFactory().builder().add("col1", subQuery.getType()).build(), + List.of(List.of(subQuery))); + + final OuterReferenceResolver resolver = resolve(virtualTable); + Assertions.assertNotNull(resolver.anchorForCorrelationId(cor0.get().id)); + } + /** * Regression test for the partially-decorrelated Filter case. * diff --git a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java index ab3a8eb2d..15702ed44 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java @@ -66,13 +66,10 @@ void expressionContainingVirtualTable() { // Check the specific Calcite encoding RelNode relNode = substraitToCalcite.convert(virtualTableScan); assertEquals( - "LogicalProject(inputs=[0..1])\n" - + " LogicalUnion(all=[true])\n" - + " LogicalProject(exprs=[[2, +(4.4E0:DOUBLE, 4.5E0:DOUBLE)]])\n" - + " LogicalValues(type=[RecordType()], tuples=[[{ }]])\n" - + " LogicalProject(exprs=[[*(6, 2), 8.8E0:DOUBLE]])\n" - + " LogicalValues(type=[RecordType()], tuples=[[{ }]])\n", + "VirtualTable(type=[RecordType(INTEGER col1, DOUBLE col2)], rows=[[{ 2, +(4.4E0:DOUBLE, 4.5E0:DOUBLE) }, { *(6, 2), 8.8E0:DOUBLE }]])\n", explain(relNode)); + + assertFullRoundTrip(virtualTableScan); } @Test @@ -171,11 +168,10 @@ void structColumnConverts() { RelNode relNode = substraitToCalcite.convert(virtualTableScan); assertEquals( - "LogicalProject(inputs=[0])\n" - + " LogicalUnion(all=[true])\n" - + " LogicalProject(exprs=[[ROW(1, 2.0E0:DOUBLE)]])\n" - + " LogicalValues(type=[RecordType()], tuples=[[{ }]])\n", + "VirtualTable(type=[RecordType(RecordType(INTEGER a, DOUBLE b) outer)], rows=[[{ ROW(1, 2.0E0:DOUBLE) }]])\n", explain(relNode)); + + assertFullRoundTrip(virtualTableScan); } /** The row after the first is where a row literal in a tuple would be compared to another. */ @@ -191,13 +187,10 @@ void twoStructRowsConvert() { RelNode relNode = substraitToCalcite.convert(virtualTableScan); assertEquals( - "LogicalProject(inputs=[0])\n" - + " LogicalUnion(all=[true])\n" - + " LogicalProject(exprs=[[ROW(1, 2.0E0:DOUBLE)]])\n" - + " LogicalValues(type=[RecordType()], tuples=[[{ }]])\n" - + " LogicalProject(exprs=[[ROW(3, 4.0E0:DOUBLE)]])\n" - + " LogicalValues(type=[RecordType()], tuples=[[{ }]])\n", + "VirtualTable(type=[RecordType(RecordType(INTEGER a, DOUBLE b) outer)], rows=[[{ ROW(1, 2.0E0:DOUBLE) }, { ROW(3, 4.0E0:DOUBLE) }]])\n", explain(relNode)); + + assertFullRoundTrip(virtualTableScan); } /** @@ -234,11 +227,10 @@ void severalStructColumnsConvert() { RelNode relNode = substraitToCalcite.convert(virtualTableScan); assertEquals( - "LogicalProject(inputs=[0..1])\n" - + " LogicalUnion(all=[true])\n" - + " LogicalProject(exprs=[[ROW(1, 2.0E0:DOUBLE), ROW('x':VARCHAR)]])\n" - + " LogicalValues(type=[RecordType()], tuples=[[{ }]])\n", + "VirtualTable(type=[RecordType(RecordType(INTEGER a, DOUBLE b) first, RecordType(VARCHAR c) second)], rows=[[{ ROW(1, 2.0E0:DOUBLE), ROW('x':VARCHAR) }]])\n", explain(relNode)); + + assertFullRoundTrip(virtualTableScan); } /** @@ -259,10 +251,7 @@ void structColumnWithAComputedField() { RelNode relNode = substraitToCalcite.convert(virtualTableScan); assertEquals( - "LogicalProject(inputs=[0])\n" - + " LogicalUnion(all=[true])\n" - + " LogicalProject(exprs=[[ROW(*(6, 2), 2.0E0:DOUBLE)]])\n" - + " LogicalValues(type=[RecordType()], tuples=[[{ }]])\n", + "VirtualTable(type=[RecordType(RecordType(INTEGER a, DOUBLE b) outer)], rows=[[{ ROW(*(6, 2), 2.0E0:DOUBLE) }]])\n", explain(relNode)); } @@ -280,10 +269,7 @@ void listColumnConverts() { RelNode relNode = substraitToCalcite.convert(virtualTableScan); assertEquals( - "LogicalProject(inputs=[0])\n" - + " LogicalUnion(all=[true])\n" - + " LogicalProject(exprs=[[ARRAY(1, 2)]])\n" - + " LogicalValues(type=[RecordType()], tuples=[[{ }]])\n", + "VirtualTable(type=[RecordType(INTEGER ARRAY col1)], rows=[[{ ARRAY(1, 2) }]])\n", explain(relNode)); } @@ -303,10 +289,7 @@ void nullableStructColumnConverts() { RelNode relNode = substraitToCalcite.convert(virtualTableScan); assertEquals( - "LogicalProject(inputs=[0])\n" - + " LogicalUnion(all=[true])\n" - + " LogicalProject(exprs=[[ROW(1, 2.0E0:DOUBLE)]])\n" - + " LogicalValues(type=[RecordType()], tuples=[[{ }]])\n", + "VirtualTable(type=[RecordType(RecordType(INTEGER a, DOUBLE b) outer)], rows=[[{ ROW(1, 2.0E0:DOUBLE) }]])\n", explain(relNode)); } @@ -396,10 +379,7 @@ void structInListColumnConverts() { RelNode relNode = substraitToCalcite.convert(virtualTableScan); assertEquals( - "LogicalProject(inputs=[0])\n" - + " LogicalUnion(all=[true])\n" - + " LogicalProject(exprs=[[ARRAY(ROW(1))]])\n" - + " LogicalValues(type=[RecordType()], tuples=[[{ }]])\n", + "VirtualTable(type=[RecordType(RecordType(INTEGER a) ARRAY col1)], rows=[[{ ARRAY(ROW(1)) }]])\n", explain(relNode)); } @@ -417,10 +397,7 @@ void structInMapColumnConverts() { RelNode relNode = substraitToCalcite.convert(virtualTableScan); assertEquals( - "LogicalProject(inputs=[0])\n" - + " LogicalUnion(all=[true])\n" - + " LogicalProject(exprs=[[MAP('k':VARCHAR, ROW(1))]])\n" - + " LogicalValues(type=[RecordType()], tuples=[[{ }]])\n", + "VirtualTable(type=[RecordType((VARCHAR, RecordType(INTEGER a)) MAP col1)], rows=[[{ MAP('k':VARCHAR, ROW(1)) }]])\n", explain(relNode)); } @@ -439,10 +416,7 @@ void nullableStructInsideStructColumnConverts() { RelNode relNode = substraitToCalcite.convert(virtualTableScan); assertEquals( - "LogicalProject(inputs=[0])\n" - + " LogicalUnion(all=[true])\n" - + " LogicalProject(exprs=[[ROW(ROW(1))]])\n" - + " LogicalValues(type=[RecordType()], tuples=[[{ }]])\n", + "VirtualTable(type=[RecordType(RecordType(RecordType(INTEGER a) inner) outer)], rows=[[{ ROW(ROW(1)) }]])\n", explain(relNode)); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java new file mode 100644 index 000000000..255b2cc7a --- /dev/null +++ b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java @@ -0,0 +1,235 @@ +package io.substrait.isthmus; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; + +import com.google.common.collect.ImmutableList; +import io.substrait.expression.Expression; +import io.substrait.isthmus.calcite.rel.VirtualTable; +import io.substrait.isthmus.calcite.rel.rules.VirtualTableExpansionRule; +import io.substrait.relation.Project; +import io.substrait.relation.Rel; +import io.substrait.relation.VirtualTableScan; +import io.substrait.type.NamedStruct; +import java.math.BigDecimal; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; +import org.apache.calcite.plan.RelOptRule; +import org.apache.calcite.plan.RelOptUtil; +import org.apache.calcite.plan.hep.HepPlanner; +import org.apache.calcite.plan.hep.HepProgramBuilder; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.logical.LogicalProject; +import org.apache.calcite.rel.logical.LogicalUnion; +import org.apache.calcite.rel.logical.LogicalValues; +import org.apache.calcite.rel.rules.CoreRules; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexCall; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.rex.RexShuttle; +import org.apache.calcite.sql.type.SqlTypeName; +import org.junit.jupiter.api.Test; + +/** + * The {@link VirtualTable} relation and the rule that expands it: what isthmus emits, what survives + * a planner, and what the expansion costs. + */ +class VirtualTableTest extends PlanTestBase { + + private final NamedStruct schema = + NamedStruct.of(List.of("col1", "col2"), R.struct(R.I32, R.FP64)); + + private VirtualTableScan computedRows() { + return virtualTable( + List.of(sb.i32(2), sb.add(sb.fp64(4.4), sb.fp64(4.5))), + List.of(sb.multiply(sb.i32(6), sb.i32(2)), sb.fp64(8.8))); + } + + @Test + void aComputedRowConvertsToTheIsthmusRelation() { + assertInstanceOf(VirtualTable.class, substraitToCalcite.convert(computedRows())); + } + + @Test + void theRuleExpandsTheRowsIntoAUnionOfProjections() { + RelNode expanded = + plan(substraitToCalcite.convert(computedRows()), VirtualTableExpansionRule.INSTANCE); + + assertEquals( + "LogicalUnion(all=[true])\n" + + " LogicalProject(col1=[2], col2=[+(4.4E0:DOUBLE, 4.5E0:DOUBLE)])\n" + + " LogicalValues(tuples=[[{ }]])\n" + + " LogicalProject(col1=[*(6, 2)], col2=[8.8E0:DOUBLE])\n" + + " LogicalValues(tuples=[[{ }]])\n", + RelOptUtil.toString(expanded)); + } + + /** One row needs no union, and a planner would strip a one-input one anyway. */ + @Test + void theRuleExpandsASingleRowIntoAProjection() { + RelNode expanded = + plan( + substraitToCalcite.convert( + virtualTable(List.of(sb.multiply(sb.i32(6), sb.i32(2)), sb.fp64(8.8)))), + VirtualTableExpansionRule.INSTANCE); + + assertEquals( + "LogicalProject(col1=[*(6, 2)], col2=[8.8E0:DOUBLE])\n" + + " LogicalValues(tuples=[[{ }]])\n", + RelOptUtil.toString(expanded)); + } + + /** + * The expansion is one-way, and this is what that costs: what comes back is the relation the + * expansion is, not the table it came from. That is the reason isthmus does not run the rule + * itself. + */ + @Test + void theExpansionDoesNotConvertBackToAVirtualTable() { + RelNode expanded = + plan(substraitToCalcite.convert(computedRows()), VirtualTableExpansionRule.INSTANCE); + + assertInstanceOf( + io.substrait.relation.Set.class, SubstraitRelVisitor.convert(expanded, extensions)); + } + + /** + * The unexpanded relation is opaque to the rules that rewrite the expansion -- UNION_REMOVE + * strips the one-input union a single-row table would expand to, and PROJECT_MERGE takes a row's + * projection into whatever sits above it -- so the table is still a table after a planner has run + * over it. + */ + @Test + void theRelationSurvivesPlanning() { + RelNode planned = + plan( + substraitToCalcite.convert(computedRows()), + CoreRules.UNION_REMOVE, + CoreRules.UNION_MERGE, + CoreRules.PROJECT_MERGE); + + assertInstanceOf(VirtualTable.class, planned); + assertEquals(computedRows(), SubstraitRelVisitor.convert(planned, extensions)); + } + + /** + * The rows are expressions, and Calcite rewrites a relation's expressions by handing it a + * shuttle. A relation that does not pass one on keeps its rows out of every rewrite built on + * that, including the scan that finds the subqueries binding outer references. + */ + @Test + void aShuttleReachesTheRows() { + RelNode table = substraitToCalcite.convert(computedRows()); + RexBuilder rexBuilder = table.getCluster().getRexBuilder(); + + RelNode rewritten = + table.accept( + new RexShuttle() { + @Override + public RexNode visitCall(RexCall call) { + return call.getOperator().getName().equals("*") + ? rexBuilder.makeExactLiteral(BigDecimal.valueOf(12), call.getType()) + : super.visitCall(call); + } + }); + + assertEquals( + "VirtualTable(rows=[[{ 2, +(4.4E0:DOUBLE, 4.5E0:DOUBLE) }, { 12, 8.8E0:DOUBLE }]])\n", + RelOptUtil.toString(rewritten)); + } + + /** + * A projection above the table is where the expansion used to lose the schema's names: the + * renaming projection it carried was merged into this one, and nothing was left to rebuild the + * table from. + */ + @Test + void theTableIsStillATableUnderAProjection() { + VirtualTableScan table = computedRows(); + Project project = + Project.builder().input(table).expressions(List.of(sb.fieldReference(table, 0))).build(); + + Rel converted = SubstraitRelVisitor.convert(substraitToCalcite.convert(project), extensions); + + assertEquals(table, assertInstanceOf(Project.class, converted).getInput()); + } + + /** + * A union someone wrote out of single-row projections is a union. It converts to the same Calcite + * tree the expansion does, so nothing in the tree can tell the two apart -- which is why the + * table is recognised by its own type instead. + */ + @Test + void aHandWrittenUnionOfSingleRowProjectionsStaysAUnion() { + RelDataType i32 = typeFactory.createSqlType(SqlTypeName.INTEGER); + RexBuilder rexBuilder = builder.getRexBuilder(); + + RelNode union = + LogicalUnion.create( + List.of( + singleRowProjection(rexBuilder.makeExactLiteral(BigDecimal.ONE, i32)), + singleRowProjection(rexBuilder.makeExactLiteral(BigDecimal.valueOf(2), i32))), + true); + + assertInstanceOf( + io.substrait.relation.Set.class, SubstraitRelVisitor.convert(union, extensions)); + } + + /** + * The same where the arms differ in nullability, which is the case a shape match cannot take: the + * rows come from the arms and the schema from the union's own row type, and the type the union + * widened to is not the type of either row. + */ + @Test + void aHandWrittenUnionOfArmsDifferingInNullabilityStaysAUnion() { + RelDataType i32 = typeFactory.createSqlType(SqlTypeName.INTEGER); + RelDataType nullableI32 = typeFactory.createTypeWithNullability(i32, true); + RexBuilder rexBuilder = builder.getRexBuilder(); + + RelNode union = + LogicalUnion.create( + List.of( + singleRowProjection(rexBuilder.makeExactLiteral(BigDecimal.ONE, i32)), + singleRowProjection(rexBuilder.makeNullLiteral(nullableI32))), + true); + + Rel converted = SubstraitRelVisitor.convert(union, extensions); + assertInstanceOf(io.substrait.relation.Set.class, converted); + assertEquals(List.of(N.I32), converted.getRecordType().fields()); + } + + /** + * A projection of one row over the single empty row, which is what an expanded row looks like. + */ + private RelNode singleRowProjection(RexNode value) { + RelDataType emptyRowType = typeFactory.createStructType(List.of(), List.of()); + RelNode emptyRow = + LogicalValues.create( + builder.getCluster(), emptyRowType, ImmutableList.of(ImmutableList.of())); + RelDataType rowType = typeFactory.builder().add("col1", value.getType()).build(); + return LogicalProject.create( + emptyRow, Collections.emptyList(), List.of(value), rowType, Collections.emptySet()); + } + + @SafeVarargs + private VirtualTableScan virtualTable(List... rows) { + List structs = + Arrays.stream(rows) + .map(row -> Expression.NestedStruct.builder().addAllFields(row).build()) + .collect(Collectors.toList()); + return VirtualTableScan.builder().initialSchema(schema).addAllRows(structs).build(); + } + + private RelNode plan(RelNode rel, RelOptRule... rules) { + HepProgramBuilder program = new HepProgramBuilder(); + for (RelOptRule rule : rules) { + program.addRuleInstance(rule); + } + HepPlanner planner = new HepPlanner(program.build()); + planner.setRoot(rel); + return planner.findBestExp(); + } +} From 86334bdcfeab9df928948960140ac934815299cd Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Fri, 28 Aug 2026 10:27:38 +0300 Subject: [PATCH 2/6] test(isthmus): cover the virtual table's two uncovered branches copy() rejecting an input and the expansion of a table with no rows were both unpinned: deleting either left the suite green. The DDL relations next door have the same input check and cover it. --- .../substrait/isthmus/VirtualTableTest.java | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java index 255b2cc7a..bb1770a6d 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java @@ -2,6 +2,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableList; import io.substrait.expression.Expression; @@ -201,6 +202,35 @@ void aHandWrittenUnionOfArmsDifferingInNullabilityStaysAUnion() { assertEquals(List.of(N.I32), converted.getRecordType().fields()); } + /** The relation stands on its own: it has no inputs, and a copy cannot give it any. */ + @Test + void theRelationTakesNoInputs() { + RelNode table = substraitToCalcite.convert(computedRows()); + + assertEquals(List.of(), table.getInputs()); + assertEquals( + RelOptUtil.toString(table), + RelOptUtil.toString(table.copy(table.getTraitSet(), List.of()))); + assertThrows( + IllegalArgumentException.class, () -> table.copy(table.getTraitSet(), List.of(table))); + } + + /** + * A table of no rows does not reach the relation through a conversion -- a virtual table with no + * rows has no row that fails to fit a tuple, so it converts to an empty {@code LogicalValues} -- + * but a consumer can build one, and the expansion of no rows is that same empty table. + */ + @Test + void theRuleExpandsATableOfNoRowsIntoAnEmptyValues() { + RelNode table = substraitToCalcite.convert(computedRows()); + RelNode empty = + new VirtualTable(table.getCluster(), table.getTraitSet(), table.getRowType(), List.of()); + + RelNode expanded = plan(empty, VirtualTableExpansionRule.INSTANCE); + + assertEquals("LogicalValues(tuples=[[]])\n", RelOptUtil.toString(expanded)); + } + /** * A projection of one row over the single empty row, which is what an expanded row looks like. */ From 6144a7a554b5a0008b41d6ac5674d287c0c4650d Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Mon, 31 Aug 2026 17:54:02 +0300 Subject: [PATCH 3/6] fix(isthmus): expand a virtual table for SQL generation, and make the relation carry its own rules --- .../isthmus/OuterReferenceResolver.java | 13 +- .../io/substrait/isthmus/RelNodeVisitor.java | 15 ++ .../isthmus/SubstraitRelNodeConverter.java | 8 +- .../isthmus/SubstraitRelVisitor.java | 25 ++- .../io/substrait/isthmus/SubstraitToSql.java | 7 +- .../isthmus/calcite/rel/VirtualTable.java | 133 ++++++++++++-- .../rel/rules/VirtualTableExpansionRule.java | 74 +++++++- .../isthmus/sql/SubstraitSqlDialect.java | 7 +- .../isthmus/OuterReferenceResolverTest.java | 5 +- .../io/substrait/isthmus/PlanTestBase.java | 43 +++++ .../isthmus/VirtualTableScanTest.java | 12 +- .../substrait/isthmus/VirtualTableTest.java | 169 ++++++++++++------ 12 files changed, 402 insertions(+), 109 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/OuterReferenceResolver.java b/isthmus/src/main/java/io/substrait/isthmus/OuterReferenceResolver.java index 877c2bbe6..6245dd6b6 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/OuterReferenceResolver.java +++ b/isthmus/src/main/java/io/substrait/isthmus/OuterReferenceResolver.java @@ -1,6 +1,5 @@ package io.substrait.isthmus; -import io.substrait.isthmus.calcite.rel.VirtualTable; import java.util.HashMap; import java.util.IdentityHashMap; import java.util.Map; @@ -116,12 +115,12 @@ public RelNode visit(Correlate correlate) throws RuntimeException { @Override public RelNode visitOther(RelNode other) throws RuntimeException { - if (other instanceof VirtualTable) { - // A virtual table's rows are expressions, and a subquery among them binds outer references - // like one anywhere else. A subquery's relation is not an input, so walking inputs never - // reaches it; Filter and Project scan their own before they get here. - other.accept(rexVisitor); - } + // A relation's own expressions can hold a subquery binding outer references, and a subquery's + // relation is not an input, so walking inputs never reaches it. Filter and Project scan theirs + // before they get here; a virtual table's rows, and a join, calc or sort condition, arrive + // here. AbstractRelNode.accept(RexShuttle) returns the relation itself, so the result is the + // one already in the tree. + other.accept(rexVisitor); for (RelNode child : other.getInputs()) { reverseAccept(child); } diff --git a/isthmus/src/main/java/io/substrait/isthmus/RelNodeVisitor.java b/isthmus/src/main/java/io/substrait/isthmus/RelNodeVisitor.java index 9a45ee40c..f05a70c4f 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/RelNodeVisitor.java +++ b/isthmus/src/main/java/io/substrait/isthmus/RelNodeVisitor.java @@ -1,5 +1,6 @@ package io.substrait.isthmus; +import io.substrait.isthmus.calcite.rel.VirtualTable; import org.apache.calcite.rel.RelNode; import org.apache.calcite.rel.core.Aggregate; import org.apache.calcite.rel.core.Calc; @@ -63,6 +64,18 @@ public OUTPUT visit(Values values) throws EXCEPTION { return visitOther(values); } + /** + * Visits a {@link VirtualTable} node, the relation isthmus converts a virtual table whose rows + * are not all literals into. + * + * @param virtualTable the virtual table node + * @return the result of visiting this node + * @throws EXCEPTION if the visit fails + */ + public OUTPUT visit(VirtualTable virtualTable) throws EXCEPTION { + return visitOther(virtualTable); + } + /** * Visits a {@link Filter} node. * @@ -261,6 +274,8 @@ public final OUTPUT reverseAccept(RelNode node) throws EXCEPTION { return this.visit((Aggregate) node); } else if (node instanceof TableModify) { return this.visit((TableModify) node); + } else if (node instanceof VirtualTable) { + return this.visit((VirtualTable) node); } else { return this.visitOther(node); } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java index d139dc63e..a03d842f6 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelNodeConverter.java @@ -63,7 +63,6 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.stream.Stream; -import org.apache.calcite.plan.Convention; import org.apache.calcite.plan.RelOptSchema; import org.apache.calcite.plan.RelOptTable; import org.apache.calcite.plan.RelOptUtil; @@ -876,12 +875,7 @@ public RelNode visit(VirtualTableScan virtualTableScan, Context context) { // consumer whose planner only knows Calcite's own relations can expand it with // VirtualTableExpansionRule. return applyRelCommon( - new VirtualTable( - relBuilder.getCluster(), - relBuilder.getCluster().traitSetOf(Convention.NONE), - rowType, - convertedRows), - virtualTableScan); + VirtualTable.create(relBuilder.getCluster(), rowType, convertedRows), virtualTableScan); } } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java index 1743fa18a..880fd8927 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java @@ -61,6 +61,7 @@ import org.apache.calcite.rel.type.RelDataTypeField; import org.apache.calcite.rex.RexBuilder; import org.apache.calcite.rex.RexInputRef; +import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; import org.apache.calcite.rex.RexUtil; import org.apache.calcite.sql.SqlKind; @@ -934,17 +935,30 @@ public Rel handleCreateView(CreateView createView) { } /** - * Handles the isthmus {@link VirtualTable}, which is what a virtual table whose rows are not all + * Converts the isthmus {@link VirtualTable}, which is what a virtual table whose rows are not all * literals converts to. * * @param virtualTable Calcite virtual table * @return Substrait virtual table scan */ - public Rel handleVirtualTable(VirtualTable virtualTable) { + @Override + public Rel visit(VirtualTable virtualTable) { + // At the row type's field types rather than the values' own, as visit(Values) does: a literal + // narrower than its column -- Calcite infers one for a tuple value, and pushes a struct's + // nullability down into its fields -- would otherwise disagree with the schema built from the + // same row type, and VirtualTableScan rejects the relation on that. + List rowFields = virtualTable.getRowType().getFieldList(); + LiteralConverter literalConverter = new LiteralConverter(typeConverter); List rows = new ArrayList<>(virtualTable.getRows().size()); for (List row : virtualTable.getRows()) { - List fields = - row.stream().map(this::toExpression).collect(Collectors.toUnmodifiableList()); + List fields = new ArrayList<>(row.size()); + for (int column = 0; column < row.size(); column++) { + RexNode value = row.get(column); + fields.add( + value instanceof RexLiteral + ? literalConverter.convert((RexLiteral) value, rowFields.get(column).getType()) + : toExpression(value)); + } rows.add(ExpressionCreator.nestedStruct(false, fields)); } return VirtualTableScan.builder() @@ -967,9 +981,6 @@ public Rel visitOther(RelNode other) { } else if (other instanceof CreateView) { return handleCreateView((CreateView) other); - - } else if (other instanceof VirtualTable) { - return handleVirtualTable((VirtualTable) other); } throw new UnsupportedOperationException("Unable to handle node: " + other); } diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java index ac07d8d65..78dc8d3f6 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitToSql.java @@ -1,6 +1,7 @@ package io.substrait.isthmus; import io.substrait.extension.SimpleExtension; +import io.substrait.isthmus.calcite.rel.rules.VirtualTableExpansionRule; import io.substrait.plan.Plan; import io.substrait.plan.Plan.Root; import io.substrait.relation.Rel; @@ -66,6 +67,8 @@ public RelNode substraitRelToCalciteRel(Rel relRoot, Prepare.CatalogReader catal * * @param plan the Substrait {@link Plan} to convert to SQL, must not be null * @param dialect the {@link SqlDialect} to generate the SQL strings for, must not be null + *

Any {@link io.substrait.isthmus.calcite.rel.VirtualTable} the conversion produced is + * expanded first: {@link RelToSqlConverter} knows Calcite's own relations only. * @return list containing a SQL string for each {@link Plan.Root} in {@code plan} */ public List convert(Plan plan, SqlDialect dialect) { @@ -75,7 +78,9 @@ public List convert(Plan plan, SqlDialect dialect) { for (Root root : plan.getRoots()) { result.add( relToSql - .visitRoot(substraitToCalcite.convert(root).project(true)) + .visitRoot( + VirtualTableExpansionRule.expandAll( + substraitToCalcite.convert(root).project(true))) .asStatement() .toSqlString(dialect) .getSql()); diff --git a/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/VirtualTable.java b/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/VirtualTable.java index 8df513991..214076e62 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/VirtualTable.java +++ b/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/VirtualTable.java @@ -1,19 +1,27 @@ package io.substrait.isthmus.calcite.rel; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import java.util.ArrayList; import java.util.List; +import java.util.Set; import java.util.stream.Collectors; +import org.apache.calcite.plan.Convention; import org.apache.calcite.plan.RelOptCluster; +import org.apache.calcite.plan.RelOptCost; +import org.apache.calcite.plan.RelOptPlanner; import org.apache.calcite.plan.RelTraitSet; import org.apache.calcite.rel.AbstractRelNode; import org.apache.calcite.rel.RelNode; import org.apache.calcite.rel.RelWriter; +import org.apache.calcite.rel.core.CorrelationId; import org.apache.calcite.rel.metadata.RelMetadataQuery; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rex.RexNode; import org.apache.calcite.rex.RexShuttle; +import org.apache.calcite.rex.RexUtil; import org.apache.calcite.sql.SqlExplainLevel; +import org.apache.calcite.util.Litmus; /** * A table of rows given as expressions, which is what a Substrait virtual table is and what Calcite @@ -24,15 +32,15 @@ * rows into stock Calcite relations -- a projection per row, unioned -- loses that: the shape a * planner leaves behind is a projection over an empty table, which is what converts back. A * consumer that needs the expansion can ask for it with {@link - * io.substrait.isthmus.calcite.rel.rules.VirtualTableExpansionRule}, knowing it is one-way; nothing - * in isthmus runs that rule. + * io.substrait.isthmus.calcite.rel.rules.VirtualTableExpansionRule}, knowing it is one-way. * *

The rows are held at the row type's own field types, names included, so that the expansion has * nothing left to derive. */ public class VirtualTable extends AbstractRelNode { - private final ImmutableList> rows; + private final ImmutableList> rows; + private final ImmutableSet variablesSet; /** * VirtualTable constructor. @@ -40,34 +48,94 @@ public class VirtualTable extends AbstractRelNode { * @param cluster the cluster this relation belongs to * @param traitSet the relation's traits * @param rowType the table's row type, carrying the schema's field names + * @param variablesSet the correlation variables the rows resolve against * @param rows one list of values per row, each value at the type its column is declared at + * @throws IllegalArgumentException if a row does not fit the row type */ public VirtualTable( RelOptCluster cluster, RelTraitSet traitSet, RelDataType rowType, + Set variablesSet, List> rows) { super(cluster, traitSet); this.rowType = rowType; - ImmutableList.Builder> builder = ImmutableList.builder(); + this.variablesSet = ImmutableSet.copyOf(variablesSet); + ImmutableList.Builder> builder = ImmutableList.builder(); for (List row : rows) { + // Nothing else checks this: the deleted LogicalProject got it from RexUtil.compatibleTypes, + // and AbstractRelNode.isValid succeeds unconditionally. + if (row.size() != rowType.getFieldCount()) { + throw new IllegalArgumentException( + String.format( + "A virtual table's row has %d values where its type declares %d columns: %s", + row.size(), rowType.getFieldCount(), row)); + } + if (!RexUtil.compatibleTypes(row, rowType, Litmus.IGNORE)) { + throw new IllegalArgumentException( + String.format( + "A virtual table's row %s does not fit the type %s its columns are declared at", + row, rowType.getFullTypeString())); + } builder.add(ImmutableList.copyOf(row)); } this.rows = builder.build(); } + /** + * Creates a virtual table with no correlation variables, in the convention every relation this + * conversion builds is in. + * + * @param cluster the cluster this relation belongs to + * @param rowType the table's row type, carrying the schema's field names + * @param rows one list of values per row + * @return the virtual table + */ + public static VirtualTable create( + RelOptCluster cluster, RelDataType rowType, List> rows) { + return create(cluster, rowType, ImmutableSet.of(), rows); + } + + /** + * Creates a virtual table whose rows resolve against the given correlation variables. + * + * @param cluster the cluster this relation belongs to + * @param rowType the table's row type, carrying the schema's field names + * @param variablesSet the correlation variables the rows resolve against + * @param rows one list of values per row + * @return the virtual table + */ + public static VirtualTable create( + RelOptCluster cluster, + RelDataType rowType, + Set variablesSet, + List> rows) { + return new VirtualTable( + cluster, cluster.traitSetOf(Convention.NONE), rowType, variablesSet, rows); + } + /** * Returns the table's rows. * * @return one list of values per row */ - public List> getRows() { + public List> getRows() { return rows; } + /** + * Returns the correlation variables the rows resolve against. + * + *

A row holding a {@link org.apache.calcite.rex.RexSubQuery} that binds an outer reference is + * unreachable by {@code SubQueryRemoveRule}, whose operands are a projection, a filter and a + * join, and by {@code RelDecorrelator}: a consumer's planner leaves it unexpanded, and the + * variables it resolves against have to travel with the relation that holds it. + * + * @return the correlation variables + */ @Override - protected RelDataType deriveRowType() { - return rowType; + public Set getVariablesSet() { + return variablesSet; } @Override @@ -75,6 +143,28 @@ public double estimateRowCount(RelMetadataQuery mq) { return rows.size(); } + /** + * Returns the cost of this relation, which is the cost of what it expands into. + * + *

The inherited cost is a row count alone, which is cheaper than the projection per row the + * expansion builds -- a cost-based planner would fire {@link + * io.substrait.isthmus.calcite.rel.rules.VirtualTableExpansionRule} and then keep the unexpanded + * relation it started from. + * + * @param planner the planner asking + * @param mq the metadata query + * @return the cost of this relation + */ + @Override + public RelOptCost computeSelfCost(RelOptPlanner planner, RelMetadataQuery mq) { + // Three relations per row -- an empty table, the projection over it, and the row's share of the + // union -- each costing its own row and the values it computes. + double relations = 3 * rows.size(); + return planner + .getCostFactory() + .makeCost(relations, relations * (rowType.getFieldCount() + 1), 0); + } + /** * Applies an expression rewrite to the rows. * @@ -82,23 +172,32 @@ public double estimateRowCount(RelMetadataQuery mq) { * not pass one on keeps its expressions out of everything built on that -- finding the subqueries * that bind outer references among them. * + *

A rewrite that retypes a value retypes the column it stands in, so the row type is rebuilt + * from the rewritten rows where they no longer carry the declared types. The names of a rebuilt + * column are the expression's own below the top level, which is the most a relation can say + * without the schema that named them. + * * @param shuttle the rewrite to apply * @return this table with the rewritten rows, or itself where nothing changed */ @Override public RelNode accept(RexShuttle shuttle) { - boolean changed = false; List> rewritten = new ArrayList<>(rows.size()); + boolean changed = false; for (List row : rows) { - List rewrittenRow = new ArrayList<>(row.size()); - for (RexNode value : row) { - RexNode visited = value.accept(shuttle); - changed |= visited != value; - rewrittenRow.add(visited); - } + List rewrittenRow = shuttle.apply(row); + changed |= rewrittenRow != row; rewritten.add(rewrittenRow); } - return changed ? new VirtualTable(getCluster(), getTraitSet(), rowType, rewritten) : this; + if (!changed) { + return this; + } + RelDataType rewrittenType = + rewritten.stream().allMatch(row -> RexUtil.compatibleTypes(row, rowType, Litmus.IGNORE)) + ? rowType + : RexUtil.createStructType( + getCluster().getTypeFactory(), rewritten.get(0), rowType.getFieldNames(), null); + return new VirtualTable(getCluster(), getTraitSet(), rewrittenType, variablesSet, rewritten); } /** @@ -111,6 +210,8 @@ public RelNode accept(RexShuttle shuttle) { public RelWriter explainTerms(RelWriter pw) { return super.explainTerms(pw) .itemIf("type", rowType, pw.getDetailLevel() == SqlExplainLevel.DIGEST_ATTRIBUTES) + .itemIf("type", rowType.getFieldList(), pw.nest()) + .itemIf("variablesSet", variablesSet, !variablesSet.isEmpty()) .item( "rows", rows.stream() @@ -135,6 +236,6 @@ public RelNode copy(RelTraitSet traitSet, List inputs) { if (!inputs.isEmpty()) { throw new IllegalArgumentException("VirtualTable takes no inputs, but got " + inputs.size()); } - return new VirtualTable(getCluster(), traitSet, rowType, rows); + return new VirtualTable(getCluster(), traitSet, rowType, variablesSet, rows); } } diff --git a/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/rules/VirtualTableExpansionRule.java b/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/rules/VirtualTableExpansionRule.java index 514630ed5..d68905849 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/rules/VirtualTableExpansionRule.java +++ b/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/rules/VirtualTableExpansionRule.java @@ -5,17 +5,22 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.stream.Collectors; import org.apache.calcite.plan.RelOptCluster; import org.apache.calcite.plan.RelOptRuleCall; import org.apache.calcite.plan.RelRule; +import org.apache.calcite.plan.hep.HepPlanner; +import org.apache.calcite.plan.hep.HepProgramBuilder; import org.apache.calcite.rel.RelNode; import org.apache.calcite.rel.core.RelFactories; import org.apache.calcite.rel.logical.LogicalProject; import org.apache.calcite.rel.logical.LogicalUnion; import org.apache.calcite.rel.logical.LogicalValues; import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeField; import org.apache.calcite.rex.RexLiteral; import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.validate.SqlValidatorUtil; import org.apache.calcite.tools.RelBuilderFactory; /** @@ -39,8 +44,21 @@ */ public class VirtualTableExpansionRule extends RelRule { - /** The rule instance to add to a planner. */ - public static final VirtualTableExpansionRule INSTANCE = Config.DEFAULT.toRule(); + /** + * Returns the rule instance to add to a planner. + * + *

Held by a nested class rather than a field of this one: the configuration builds a rule and + * the rule reads its configuration, so a field here would have the two initialize each other. + * + * @return the rule instance + */ + public static VirtualTableExpansionRule instance() { + return InstanceHolder.INSTANCE; + } + + private static final class InstanceHolder { + private static final VirtualTableExpansionRule INSTANCE = Config.DEFAULT.toRule(); + } private VirtualTableExpansionRule(Config config) { super(config); @@ -51,6 +69,23 @@ public void onMatch(RelOptRuleCall call) { call.transformTo(expand(call.rel(0))); } + /** + * Expands every {@link VirtualTable} in the given tree, leaving the rest of it alone. + * + *

For a consumer that has to hand the tree to something knowing only Calcite's own relations. + * Isthmus' own SQL generation is one: {@link org.apache.calcite.rel.rel2sql.RelToSqlConverter} + * has no case for a relation it does not know and throws an {@link AssertionError} naming it. + * + * @param relNode the tree to expand + * @return the tree with every virtual table expanded + */ + public static RelNode expandAll(RelNode relNode) { + HepPlanner planner = + new HepPlanner(new HepProgramBuilder().addRuleInstance(instance()).build()); + planner.setRoot(relNode); + return planner.findBestExp(); + } + private static RelNode expand(VirtualTable virtualTable) { RelOptCluster cluster = virtualTable.getCluster(); RelDataType rowType = virtualTable.getRowType(); @@ -60,13 +95,34 @@ private static RelNode expand(VirtualTable virtualTable) { RelDataType emptyRowType = cluster.getTypeFactory().createStructType(List.of(), List.of()); ImmutableList> singleEmptyRow = ImmutableList.of(ImmutableList.of()); + // One empty row for every projection: they share a digest, so a planner keeps one of them + // whether the rule builds one or many. + RelNode emptyRow = LogicalValues.create(cluster, emptyRowType, singleEmptyRow); + + // A projection carries the variables the rows resolve against: a subquery among them is + // unreachable by SubQueryRemoveRule and RelDecorrelator, and the expansion is what a consumer + // hands to a planner that only knows Calcite's own relations. + List fieldNames = + SqlValidatorUtil.uniquify( + rowType.getFieldNames(), SqlValidatorUtil.F_SUGGESTER, /* caseSensitive= */ true); + RelDataType projectRowType = + cluster + .getTypeFactory() + .createStructType( + rowType.getFieldList().stream() + .map(RelDataTypeField::getType) + .collect(Collectors.toList()), + fieldNames); List rowProjects = new ArrayList<>(); for (List row : virtualTable.getRows()) { - RelNode emptyRow = LogicalValues.create(cluster, emptyRowType, singleEmptyRow); rowProjects.add( LogicalProject.create( - emptyRow, Collections.emptyList(), row, rowType, Collections.emptySet())); + emptyRow, + Collections.emptyList(), + row, + projectRowType, + virtualTable.getVariablesSet())); } // A one-input union is not a relation a planner keeps -- UNION_REMOVE strips it -- and the // projection is what the expansion means anyway. @@ -76,15 +132,19 @@ private static RelNode expand(VirtualTable virtualTable) { /** * Rule configuration. * - *

Written out rather than generated: the rule matches one relation and has nothing to - * configure, so the three properties {@link RelRule.Config} declares are all there is. + *

Written out rather than generated: the generated implementation copies {@link + * RelRule.Config#description()}, which Calcite declares nullable, and so imports {@code + * javax.annotation.Nullable} -- which isthmus does not have on its compile classpath. + * + *

{@link #relBuilderFactory()} is inert: the expansion is built directly and never asks the + * call for a builder. The interface requires an answer, so this is Calcite's own default. */ public static class Config implements RelRule.Config { private static final OperandTransform TABLE = operand -> operand.operand(VirtualTable.class).noInputs(); - /** The configuration {@link VirtualTableExpansionRule#INSTANCE} is built from. */ + /** The configuration {@link VirtualTableExpansionRule#instance()} is built from. */ public static final Config DEFAULT = new Config(RelFactories.LOGICAL_BUILDER, "VirtualTableExpansionRule", TABLE); diff --git a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlDialect.java b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlDialect.java index e4b19b4d0..45c91783d 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlDialect.java +++ b/isthmus/src/main/java/io/substrait/isthmus/sql/SubstraitSqlDialect.java @@ -1,5 +1,6 @@ package io.substrait.isthmus.sql; +import io.substrait.isthmus.calcite.rel.rules.VirtualTableExpansionRule; import org.apache.calcite.rel.RelNode; import org.apache.calcite.rel.rel2sql.RelToSqlConverter; import org.apache.calcite.sql.SqlDialect; @@ -23,12 +24,16 @@ public class SubstraitSqlDialect extends SqlDialect { /** * Converts a Calcite {@link RelNode} to its SQL representation using the default dialect. * + *

Any {@link io.substrait.isthmus.calcite.rel.VirtualTable} in the tree is expanded first: + * {@link RelToSqlConverter} knows Calcite's own relations only. + * * @param relNode The Calcite relational node to convert. * @return A {@link SqlString} representing the SQL equivalent of the given {@link RelNode}. */ public static SqlString toSql(RelNode relNode) { RelToSqlConverter relToSql = new RelToSqlConverter(DEFAULT); - SqlNode sqlNode = relToSql.visitRoot(relNode).asStatement(); + SqlNode sqlNode = + relToSql.visitRoot(VirtualTableExpansionRule.expandAll(relNode)).asStatement(); return sqlNode.toSqlString( c -> c.withAlwaysUseParentheses(false) diff --git a/isthmus/src/test/java/io/substrait/isthmus/OuterReferenceResolverTest.java b/isthmus/src/test/java/io/substrait/isthmus/OuterReferenceResolverTest.java index c24cd7419..0527f8367 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/OuterReferenceResolverTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/OuterReferenceResolverTest.java @@ -2,7 +2,6 @@ import io.substrait.isthmus.calcite.rel.VirtualTable; import java.util.List; -import org.apache.calcite.plan.Convention; import org.apache.calcite.rel.RelNode; import org.apache.calcite.rel.core.JoinRelType; import org.apache.calcite.rex.RexCorrelVariable; @@ -157,10 +156,10 @@ void correlationInsideASubqueryInAVirtualTableRow() { final RexNode subQuery = RexSubQuery.scalar(correlated); final RelNode virtualTable = - new VirtualTable( + VirtualTable.create( tpcDsRelBuilder.getCluster(), - tpcDsRelBuilder.getCluster().traitSetOf(Convention.NONE), tpcDsRelBuilder.getTypeFactory().builder().add("col1", subQuery.getType()).build(), + java.util.Set.of(cor0.get().id), List.of(List.of(subQuery))); final OuterReferenceResolver resolver = resolve(virtualTable); diff --git a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java index c560b29b7..3c47a2c83 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java +++ b/isthmus/src/test/java/io/substrait/isthmus/PlanTestBase.java @@ -19,6 +19,8 @@ import io.substrait.relation.ProtoRelConverter; import io.substrait.relation.Rel; import io.substrait.relation.RelProtoConverter; +import io.substrait.relation.VirtualTableScan; +import io.substrait.type.NamedStruct; import io.substrait.type.Type; import io.substrait.type.TypeCreator; import java.io.IOException; @@ -27,6 +29,9 @@ import java.util.List; import org.apache.calcite.adapter.tpcds.TpcdsSchema; import org.apache.calcite.jdbc.CalciteSchema; +import org.apache.calcite.plan.RelOptRule; +import org.apache.calcite.plan.hep.HepPlanner; +import org.apache.calcite.plan.hep.HepProgramBuilder; import org.apache.calcite.prepare.CalciteCatalogReader; import org.apache.calcite.prepare.Prepare; import org.apache.calcite.rel.RelNode; @@ -369,6 +374,44 @@ protected String toSql(Plan plan) { return SubstraitSqlDialect.toSql(project).getSql(); } + /** + * Builds a virtual table of the given rows at the given schema. + * + * @param schema the table's schema + * @param rows one list of values per row + * @return the virtual table scan + */ + @SafeVarargs + protected final VirtualTableScan virtualTable( + NamedStruct schema, List... rows) { + List structs = + java.util.Arrays.stream(rows) + .map( + row -> + io.substrait.expression.Expression.NestedStruct.builder() + .addAllFields(row) + .build()) + .collect(java.util.stream.Collectors.toList()); + return VirtualTableScan.builder().initialSchema(schema).addAllRows(structs).build(); + } + + /** + * Runs the given rules over the given tree, exhaustively and in order. + * + * @param rel the tree to plan + * @param rules the rules to run + * @return the planned tree + */ + protected RelNode plan(RelNode rel, RelOptRule... rules) { + HepProgramBuilder program = new HepProgramBuilder(); + for (RelOptRule rule : rules) { + program.addRuleInstance(rule); + } + HepPlanner planner = new HepPlanner(program.build()); + planner.setRoot(rel); + return planner.findBestExp(); + } + protected io.substrait.proto.Plan toProto(Plan plan) { return new PlanProtoConverter().toProto(plan); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java index 15702ed44..347dc440d 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java @@ -253,6 +253,8 @@ void structColumnWithAComputedField() { assertEquals( "VirtualTable(type=[RecordType(RecordType(INTEGER a, DOUBLE b) outer)], rows=[[{ ROW(*(6, 2), 2.0E0:DOUBLE) }]])\n", explain(relNode)); + + assertFullRoundTrip(virtualTableScan); } /** @@ -271,6 +273,8 @@ void listColumnConverts() { assertEquals( "VirtualTable(type=[RecordType(INTEGER ARRAY col1)], rows=[[{ ARRAY(1, 2) }]])\n", explain(relNode)); + + assertFullRoundTrip(virtualTableScan); } /** @@ -381,6 +385,8 @@ void structInListColumnConverts() { assertEquals( "VirtualTable(type=[RecordType(RecordType(INTEGER a) ARRAY col1)], rows=[[{ ARRAY(ROW(1)) }]])\n", explain(relNode)); + + assertFullRoundTrip(virtualTableScan); } /** The same one level down inside a map, where the names reach a key and a value alike. */ @@ -399,11 +405,15 @@ void structInMapColumnConverts() { assertEquals( "VirtualTable(type=[RecordType((VARCHAR, RecordType(INTEGER a)) MAP col1)], rows=[[{ MAP('k':VARCHAR, ROW(1)) }]])\n", explain(relNode)); + + assertFullRoundTrip(virtualTableScan); } /** * A nullable struct nested in a column: renaming it gives back a ROW call rather than a literal, - * so the struct around it cannot be rebuilt as a literal either. + * so the struct around it cannot be rebuilt as a literal either. Pinned as a conversion rather + * than a round trip for the same reason as its sibling above: Calcite pushes a struct's + * nullability down into its fields, so the schema comes back with nullable fields. */ @Test void nullableStructInsideStructColumnConverts() { diff --git a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java index bb1770a6d..c3d5005d2 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableTest.java @@ -1,30 +1,28 @@ package io.substrait.isthmus; +import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertThrows; import com.google.common.collect.ImmutableList; -import io.substrait.expression.Expression; import io.substrait.isthmus.calcite.rel.VirtualTable; import io.substrait.isthmus.calcite.rel.rules.VirtualTableExpansionRule; +import io.substrait.isthmus.sql.SubstraitSqlDialect; import io.substrait.relation.Project; import io.substrait.relation.Rel; import io.substrait.relation.VirtualTableScan; import io.substrait.type.NamedStruct; import java.math.BigDecimal; -import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.stream.Collectors; -import org.apache.calcite.plan.RelOptRule; import org.apache.calcite.plan.RelOptUtil; -import org.apache.calcite.plan.hep.HepPlanner; -import org.apache.calcite.plan.hep.HepProgramBuilder; import org.apache.calcite.rel.RelNode; import org.apache.calcite.rel.logical.LogicalProject; import org.apache.calcite.rel.logical.LogicalUnion; import org.apache.calcite.rel.logical.LogicalValues; +import org.apache.calcite.rel.metadata.RelMetadataQuery; import org.apache.calcite.rel.rules.CoreRules; import org.apache.calcite.rel.type.RelDataType; import org.apache.calcite.rex.RexBuilder; @@ -45,6 +43,7 @@ class VirtualTableTest extends PlanTestBase { private VirtualTableScan computedRows() { return virtualTable( + schema, List.of(sb.i32(2), sb.add(sb.fp64(4.4), sb.fp64(4.5))), List.of(sb.multiply(sb.i32(6), sb.i32(2)), sb.fp64(8.8))); } @@ -57,7 +56,7 @@ void aComputedRowConvertsToTheIsthmusRelation() { @Test void theRuleExpandsTheRowsIntoAUnionOfProjections() { RelNode expanded = - plan(substraitToCalcite.convert(computedRows()), VirtualTableExpansionRule.INSTANCE); + plan(substraitToCalcite.convert(computedRows()), VirtualTableExpansionRule.instance()); assertEquals( "LogicalUnion(all=[true])\n" @@ -74,8 +73,8 @@ void theRuleExpandsASingleRowIntoAProjection() { RelNode expanded = plan( substraitToCalcite.convert( - virtualTable(List.of(sb.multiply(sb.i32(6), sb.i32(2)), sb.fp64(8.8)))), - VirtualTableExpansionRule.INSTANCE); + virtualTable(schema, List.of(sb.multiply(sb.i32(6), sb.i32(2)), sb.fp64(8.8)))), + VirtualTableExpansionRule.instance()); assertEquals( "LogicalProject(col1=[*(6, 2)], col2=[8.8E0:DOUBLE])\n" @@ -91,10 +90,10 @@ void theRuleExpandsASingleRowIntoAProjection() { @Test void theExpansionDoesNotConvertBackToAVirtualTable() { RelNode expanded = - plan(substraitToCalcite.convert(computedRows()), VirtualTableExpansionRule.INSTANCE); + plan(substraitToCalcite.convert(computedRows()), VirtualTableExpansionRule.instance()); assertInstanceOf( - io.substrait.relation.Set.class, SubstraitRelVisitor.convert(expanded, extensions)); + io.substrait.relation.Set.class, SubstraitRelVisitor.convert(expanded, converterProvider)); } /** @@ -105,15 +104,19 @@ void theExpansionDoesNotConvertBackToAVirtualTable() { */ @Test void theRelationSurvivesPlanning() { + VirtualTableScan table = computedRows(); + Project project = + Project.builder().input(table).expressions(List.of(sb.fieldReference(table, 0))).build(); + RelNode planned = plan( - substraitToCalcite.convert(computedRows()), + substraitToCalcite.convert(project), CoreRules.UNION_REMOVE, CoreRules.UNION_MERGE, CoreRules.PROJECT_MERGE); - assertInstanceOf(VirtualTable.class, planned); - assertEquals(computedRows(), SubstraitRelVisitor.convert(planned, extensions)); + Rel converted = SubstraitRelVisitor.convert(planned, converterProvider); + assertEquals(table, assertInstanceOf(Project.class, converted).getInput()); } /** @@ -153,32 +156,12 @@ void theTableIsStillATableUnderAProjection() { Project project = Project.builder().input(table).expressions(List.of(sb.fieldReference(table, 0))).build(); - Rel converted = SubstraitRelVisitor.convert(substraitToCalcite.convert(project), extensions); + Rel converted = + SubstraitRelVisitor.convert(substraitToCalcite.convert(project), converterProvider); assertEquals(table, assertInstanceOf(Project.class, converted).getInput()); } - /** - * A union someone wrote out of single-row projections is a union. It converts to the same Calcite - * tree the expansion does, so nothing in the tree can tell the two apart -- which is why the - * table is recognised by its own type instead. - */ - @Test - void aHandWrittenUnionOfSingleRowProjectionsStaysAUnion() { - RelDataType i32 = typeFactory.createSqlType(SqlTypeName.INTEGER); - RexBuilder rexBuilder = builder.getRexBuilder(); - - RelNode union = - LogicalUnion.create( - List.of( - singleRowProjection(rexBuilder.makeExactLiteral(BigDecimal.ONE, i32)), - singleRowProjection(rexBuilder.makeExactLiteral(BigDecimal.valueOf(2), i32))), - true); - - assertInstanceOf( - io.substrait.relation.Set.class, SubstraitRelVisitor.convert(union, extensions)); - } - /** * The same where the arms differ in nullability, which is the case a shape match cannot take: the * rows come from the arms and the schema from the union's own row type, and the type the union @@ -197,7 +180,7 @@ void aHandWrittenUnionOfArmsDifferingInNullabilityStaysAUnion() { singleRowProjection(rexBuilder.makeNullLiteral(nullableI32))), true); - Rel converted = SubstraitRelVisitor.convert(union, extensions); + Rel converted = SubstraitRelVisitor.convert(union, converterProvider); assertInstanceOf(io.substrait.relation.Set.class, converted); assertEquals(List.of(N.I32), converted.getRecordType().fields()); } @@ -223,14 +206,101 @@ void theRelationTakesNoInputs() { @Test void theRuleExpandsATableOfNoRowsIntoAnEmptyValues() { RelNode table = substraitToCalcite.convert(computedRows()); - RelNode empty = - new VirtualTable(table.getCluster(), table.getTraitSet(), table.getRowType(), List.of()); + RelNode empty = VirtualTable.create(table.getCluster(), table.getRowType(), List.of()); - RelNode expanded = plan(empty, VirtualTableExpansionRule.INSTANCE); + RelNode expanded = plan(empty, VirtualTableExpansionRule.instance()); assertEquals("LogicalValues(tuples=[[]])\n", RelOptUtil.toString(expanded)); } + /** + * SQL generation is the consumer the rule exists for: {@link + * org.apache.calcite.rel.rel2sql.RelToSqlConverter} knows Calcite's own relations only, and + * throws an {@code AssertionError} naming anything else -- unconditionally, so assertions being + * off does not help. Both of isthmus' entry points expand before they convert. + */ + @Test + void sqlGenerationExpandsTheTable() { + RelNode table = substraitToCalcite.convert(computedRows()); + io.substrait.plan.Plan plan = sb.plan(sb.root(computedRows(), List.of("col1", "col2"))); + + assertAll( + () -> + assertEquals( + "SELECT 2 AS \"col1\", 4.4E0 + 4.5E0 AS \"col2\"\n" + + "FROM (VALUES ()) AS \"t\"\n" + + "UNION ALL\n" + + "SELECT 6 * 2 AS \"col1\", 8.8E0 AS \"col2\"\n" + + "FROM (VALUES ()) AS \"t\"", + SubstraitSqlDialect.toSql(table).getSql()), + () -> + assertEquals( + 1, + new SubstraitToSql(converterProvider) + .convert(plan, SubstraitSqlDialect.DEFAULT) + .size())); + } + + /** + * The rows have to fit the row type: the projection this used to be built as gave that check for + * free through {@code RexUtil.compatibleTypes}, and {@code AbstractRelNode.isValid} succeeds + * whatever it is handed. + */ + @Test + void theRowsHaveToFitTheRowType() { + RelNode table = substraitToCalcite.convert(computedRows()); + RelDataType rowType = table.getRowType(); + RexBuilder rexBuilder = table.getCluster().getRexBuilder(); + RexNode i32 = + rexBuilder.makeExactLiteral(BigDecimal.ONE, typeFactory.createSqlType(SqlTypeName.INTEGER)); + + assertAll( + () -> + assertThrows( + IllegalArgumentException.class, + () -> VirtualTable.create(table.getCluster(), rowType, List.of(List.of(i32)))), + () -> + assertThrows( + IllegalArgumentException.class, + () -> + VirtualTable.create(table.getCluster(), rowType, List.of(List.of(i32, i32))))); + } + + /** + * A schema may name two columns the same -- the spec asks only that the names are a depth-first + * list -- and the table carries them as they are. The expansion cannot: a Calcite projection + * requires distinct names, so it uniquifies them there rather than failing on a table that + * converts and round-trips. + */ + @Test + void theExpansionUniquifiesRepeatedFieldNames() { + NamedStruct repeated = NamedStruct.of(List.of("c", "c"), R.struct(R.I32, R.FP64)); + RelNode table = + substraitToCalcite.convert( + virtualTable(repeated, List.of(sb.i32(2), sb.add(sb.fp64(4.4), sb.fp64(4.5))))); + + assertEquals(List.of("c", "c"), table.getRowType().getFieldNames()); + assertEquals( + List.of("c", "c1"), + plan(table, VirtualTableExpansionRule.instance()).getRowType().getFieldNames()); + } + + /** + * The relation costs what its expansion costs. The inherited estimate is a row count alone, which + * is less than the projection per row the rule builds, so a cost-based planner would fire the + * rule and then keep the relation it started from. + */ + @Test + void theRelationCostsWhatItsExpansionCosts() { + RelNode table = substraitToCalcite.convert(computedRows()); + RelNode expanded = plan(table, VirtualTableExpansionRule.instance()); + RelMetadataQuery mq = table.getCluster().getMetadataQuery(); + + assertFalse( + mq.getCumulativeCost(table).isLt(mq.getCumulativeCost(expanded)), + mq.getCumulativeCost(table) + " < " + mq.getCumulativeCost(expanded)); + } + /** * A projection of one row over the single empty row, which is what an expanded row looks like. */ @@ -243,23 +313,4 @@ private RelNode singleRowProjection(RexNode value) { return LogicalProject.create( emptyRow, Collections.emptyList(), List.of(value), rowType, Collections.emptySet()); } - - @SafeVarargs - private VirtualTableScan virtualTable(List... rows) { - List structs = - Arrays.stream(rows) - .map(row -> Expression.NestedStruct.builder().addAllFields(row).build()) - .collect(Collectors.toList()); - return VirtualTableScan.builder().initialSchema(schema).addAllRows(structs).build(); - } - - private RelNode plan(RelNode rel, RelOptRule... rules) { - HepProgramBuilder program = new HepProgramBuilder(); - for (RelOptRule rule : rules) { - program.addRuleInstance(rule); - } - HepPlanner planner = new HepPlanner(program.build()); - planner.setRoot(rel); - return planner.findBestExp(); - } } From 9c8abecd01ca5fa76c63fca28e34cca05841195a Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Mon, 31 Aug 2026 23:02:00 +0300 Subject: [PATCH 4/6] fix(isthmus): convert a virtual table's values at the types its columns declare A literal is converted at its column's declared type, the way visit(Values) does it, so a value Calcite inferred a narrower type for does not disagree with the schema built from the same row type. A value that is not a literal cannot be given that type: it is converted from the expressions it is built of and takes its type from them, and casting at the declared type would put an expression in the output the input did not have. That shape is refused here, naming the value and both types, rather than left to VirtualTableScan, whose check compares the two without promoting either. A computed field inside a nullable struct is where the two meet: Calcite pushes the struct's nullability into its fields, so the schema taken from the row type has nullable fields while the expression keeps its own. --- .../isthmus/SubstraitRelVisitor.java | 20 +++++++++++--- .../isthmus/VirtualTableScanTest.java | 26 +++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java index 880fd8927..46b05965b 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java +++ b/isthmus/src/main/java/io/substrait/isthmus/SubstraitRelVisitor.java @@ -954,10 +954,24 @@ public Rel visit(VirtualTable virtualTable) { List fields = new ArrayList<>(row.size()); for (int column = 0; column < row.size(); column++) { RexNode value = row.get(column); - fields.add( + RelDataType declaredType = rowFields.get(column).getType(); + Expression converted = value instanceof RexLiteral - ? literalConverter.convert((RexLiteral) value, rowFields.get(column).getType()) - : toExpression(value)); + ? literalConverter.convert((RexLiteral) value, declaredType) + : toExpression(value); + // A value that is not a literal is converted from the expressions it is built of and + // takes its type from them, which the declared type cannot be put back on: casting at it + // would put an expression in the output the input did not have. Refused here rather than + // left to VirtualTableScan, whose check compares the two types without promoting either. + Type declared = typeConverter.toSubstrait(declaredType); + if (!converted.getType().equals(declared)) { + throw new UnsupportedOperationException( + String.format( + "A virtual table's value %s converts to %s where its column is declared %s: " + + "isthmus cannot convert a value that does not carry its column's type", + value, converted.getType(), declared)); + } + fields.add(converted); } rows.add(ExpressionCreator.nestedStruct(false, fields)); } diff --git a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java index 347dc440d..454a05956 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/VirtualTableScanTest.java @@ -409,6 +409,32 @@ void structInMapColumnConverts() { assertFullRoundTrip(virtualTableScan); } + /** + * A computed field inside a nullable struct is where the row type stops being able to say what + * the schema said: Calcite pushes the struct's nullability into its fields, and a value built + * from expressions takes its type from them, so the trip back cannot rebuild the declared type. + * Reported here rather than as a type mismatch from {@link VirtualTableScan}'s own check. + */ + @Test + void aComputedFieldInsideANullableStructIsReportedOnTheWayBack() { + NamedStruct schema = + NamedStruct.of(List.of("outer", "a", "b"), R.struct(N.struct(R.I32, R.FP64))); + VirtualTableScan virtualTableScan = + createVirtualTableScan( + schema, + List.of( + ExpressionCreator.nestedStruct( + true, List.of(sb.multiply(sb.i32(6), sb.i32(2)), sb.fp64(2.0))))); + RelNode relNode = substraitToCalcite.convert(virtualTableScan); + + assertTrue( + assertThrows( + UnsupportedOperationException.class, + () -> SubstraitRelVisitor.convert(relNode, converterProvider)) + .getMessage() + .contains("does not carry its column's type")); + } + /** * A nullable struct nested in a column: renaming it gives back a ROW call rather than a literal, * so the struct around it cannot be rebuilt as a literal either. Pinned as a conversion rather From 6ac60687eb1ec15d624a5b00fc544359354d7561 Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Mon, 31 Aug 2026 23:02:00 +0300 Subject: [PATCH 5/6] test(isthmus): reach a correlation declared inside a virtual table's row A subquery's relation is not an input, so walking inputs never reaches a correlation it declares; the row walk added here is what does. The test puts a subquery in a row of a virtual table that is the right input of a correlate: the enclosing relation binds its own id, and the filter inside the row's subquery binds a second one that only the row walk reaches. Removing the walk fails it, and nothing else. The table declares no correlation variables of its own -- an id binds to a relation whose fields the reference names, and a leaf has none -- which its accessor now says. --- .../isthmus/calcite/rel/VirtualTable.java | 4 ++ .../isthmus/OuterReferenceResolverTest.java | 69 +++++++++++++------ 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/VirtualTable.java b/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/VirtualTable.java index 214076e62..4aaf2d888 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/VirtualTable.java +++ b/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/VirtualTable.java @@ -131,6 +131,10 @@ public List> getRows() { * join, and by {@code RelDecorrelator}: a consumer's planner leaves it unexpanded, and the * variables it resolves against have to travel with the relation that holds it. * + *

Only a consumer populates it. A conversion never does: an id is bound to a relation whose + * fields the reference names, and a leaf with no inputs has none, so an outer reference in a row + * belongs to the relation around the table the way one in a projection's expression does. + * * @return the correlation variables */ @Override diff --git a/isthmus/src/test/java/io/substrait/isthmus/OuterReferenceResolverTest.java b/isthmus/src/test/java/io/substrait/isthmus/OuterReferenceResolverTest.java index 0527f8367..0f9e78d6b 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/OuterReferenceResolverTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/OuterReferenceResolverTest.java @@ -135,35 +135,64 @@ void nestedApplyJoinQuery() throws SqlParseException { /** * A virtual table's rows are expressions, and a subquery among them is a tree the resolver only * reaches through the row: a subquery's relation is not an input, so walking inputs never gets - * there and a correlation declared inside it is left unbound. + * there and a correlation the subquery declares is left unbound. + * + *

The table itself declares nothing. It is a leaf with no inputs and so no fields to bind an + * id to, which puts a row's outer reference where one in a {@link + * org.apache.calcite.rel.core.Project} expression sits: bound by the relation around it, here the + * correlate the table is the right input of. */ @Test void correlationInsideASubqueryInAVirtualTableRow() { final Holder cor0 = Holder.empty(); - final RelNode correlated = - tpcDsRelBuilder - .scan("tpcds", "STORE_SALES") - .variable(cor0::set) - .scan("tpcds", "ITEM") - .filter( - tpcDsRelBuilder.equals( - tpcDsRelBuilder.field("I_ITEM_SK"), - tpcDsRelBuilder.field(cor0.get(), "SS_ITEM_SK"))) - .project(tpcDsRelBuilder.field("I_ITEM_SK")) - .correlate(JoinRelType.INNER, cor0.get().id, tpcDsRelBuilder.field(2, 0, "SS_ITEM_SK")) - .project(tpcDsRelBuilder.field("SS_ITEM_SK")) - .build(); - final RexNode subQuery = RexSubQuery.scalar(correlated); + final Holder cor1 = Holder.empty(); + tpcDsRelBuilder.scan("tpcds", "STORE_SALES").variable(cor0::set); + + // SELECT i_item_sk FROM item + // WHERE i_item_sk = $cor0.ss_item_sk + // AND i_item_sk = (SELECT p_promo_sk FROM promotion WHERE p_item_sk = item.i_item_sk) + tpcDsRelBuilder.scan("tpcds", "ITEM").variable(cor1::set); + final RexNode promoOfItem = + RexSubQuery.scalar( + tpcDsRelBuilder + .scan("tpcds", "PROMOTION") + .filter( + tpcDsRelBuilder.equals( + tpcDsRelBuilder.field("P_ITEM_SK"), + tpcDsRelBuilder.field(cor1.get(), "I_ITEM_SK"))) + .project(tpcDsRelBuilder.field("P_PROMO_SK")) + .build()); + final RexNode itemOfSale = + RexSubQuery.scalar( + tpcDsRelBuilder + .filter( + List.of(cor1.get().id), + tpcDsRelBuilder.equals( + tpcDsRelBuilder.field("I_ITEM_SK"), + tpcDsRelBuilder.field(cor0.get(), "SS_ITEM_SK")), + tpcDsRelBuilder.equals(tpcDsRelBuilder.field("I_ITEM_SK"), promoOfItem)) + .project(tpcDsRelBuilder.field("I_ITEM_SK")) + .build()); final RelNode virtualTable = VirtualTable.create( tpcDsRelBuilder.getCluster(), - tpcDsRelBuilder.getTypeFactory().builder().add("col1", subQuery.getType()).build(), - java.util.Set.of(cor0.get().id), - List.of(List.of(subQuery))); + tpcDsRelBuilder.getTypeFactory().builder().add("col1", itemOfSale.getType()).build(), + List.of(List.of(itemOfSale))); + final RelNode calciteRel = + tpcDsRelBuilder + .push(virtualTable) + .correlate(JoinRelType.INNER, cor0.get().id, tpcDsRelBuilder.field(2, 0, "SS_ITEM_SK")) + .build(); - final OuterReferenceResolver resolver = resolve(virtualTable); - Assertions.assertNotNull(resolver.anchorForCorrelationId(cor0.get().id)); + final OuterReferenceResolver resolver = resolve(calciteRel); + final Integer anchor0 = resolver.anchorForCorrelationId(cor0.get().id); + // Bound by the filter inside the row's subquery, which nothing but the row walk reaches. + final Integer anchor1 = resolver.anchorForCorrelationId(cor1.get().id); + + Assertions.assertNotNull(anchor0); + Assertions.assertNotNull(anchor1); + Assertions.assertNotEquals(anchor0, anchor1); } /** From 21e77683e90f39549562dc5010e333ff71cd54df Mon Sep 17 00:00:00 2001 From: Aleksandr Efimov Date: Mon, 31 Aug 2026 23:02:00 +0300 Subject: [PATCH 6/6] docs(isthmus): say what a generated rule configuration runs into The generated implementation carries javax.annotation.Nullable for the nullable description RelRule.Config declares, in the builder method that copies from the supertype. Value.Style's allowedClasspathAnnotations, nullableAnnotation and fallbackNullableAnnotation do not reach it there, which is worth saying where the hand-written configuration is. --- .../isthmus/calcite/rel/rules/VirtualTableExpansionRule.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/rules/VirtualTableExpansionRule.java b/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/rules/VirtualTableExpansionRule.java index d68905849..6f888dcbb 100644 --- a/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/rules/VirtualTableExpansionRule.java +++ b/isthmus/src/main/java/io/substrait/isthmus/calcite/rel/rules/VirtualTableExpansionRule.java @@ -134,7 +134,10 @@ private static RelNode expand(VirtualTable virtualTable) { * *

Written out rather than generated: the generated implementation copies {@link * RelRule.Config#description()}, which Calcite declares nullable, and so imports {@code - * javax.annotation.Nullable} -- which isthmus does not have on its compile classpath. + * javax.annotation.Nullable} -- which isthmus does not have on its compile classpath. The + * annotation lands in the builder method that copies from the supertype, where {@code + * Value.Style}'s {@code allowedClasspathAnnotations}, {@code nullableAnnotation} and {@code + * fallbackNullableAnnotation} do not reach it. * *

{@link #relBuilderFactory()} is inert: the expansion is built directly and never asks the * call for a builder. The interface requires an answer, so this is Calcite's own default.