Skip to content

refactor(isthmus): visit(VirtualTableScan) builds one empty LogicalValues per row and two parallel value lists #1218

Description

@nielspardon

Two allocations in visit(VirtualTableScan) scale with the table for no gain.

A fresh empty LogicalValues is built inside the per-row loop, although emptyRowType and emptyRowValue are hoisted above it and identical for every row:

RelDataType emptyRowType = typeFactory.createStructType(List.of(), List.of());
ImmutableList<ImmutableList<RexLiteral>> emptyRowValue = ImmutableList.of(ImmutableList.of());
List<RelNode> projects = new ArrayList<>();
for (final List<RexNode> rexRow : convertedRows) {
RelNode values = LogicalValues.create(relBuilder.getCluster(), emptyRowType, emptyRowValue);
RelNode project =
LogicalProject.create(
values, Collections.emptyList(), rexRow, rowType, Collections.emptySet());
projects.add(project);
}

An N-row table therefore gets N identical single-empty-tuple nodes, each computing its own digest. Calcite RelNodes are immutable and may have several parents, so one instance above the loop serves every projection.

And both value lists are filled for every value of every row, while the branch below reads exactly one of them and discards the other:

List<List<RexNode>> convertedRows = new ArrayList<>();
// The same values with the cast a nullable literal converts as taken off, which is the form a
// LogicalValues tuple would hold them in -- where they are literals at all.
List<List<RexNode>> tupleValues = new ArrayList<>();
for (final Expression.NestedStruct rowExpr : virtualTableScan.getRows()) {
List<RexNode> convertedRow = new ArrayList<>();
List<RexNode> tupleRow = new ArrayList<>();
for (int column = 0; column < rowExpr.fields().size(); column++) {
Expression field = rowExpr.fields().get(column);
RelDataType declaredType = rowType.getFieldList().get(column).getType();
RexNode value =
valueAsDeclared(field.accept(expressionRexConverter, context), declaredType);
convertedRow.add(value);
// Only a converted literal has its nullability cast taken off. A cast the plan carries
// itself is doing work -- it has a failure behavior, and it is part of what round-trips --
// so a row holding one is computed rather than tabulated.
tupleRow.add(field instanceof Expression.Literal ? unwrapNullabilityCast(value) : value);
}
convertedRows.add(convertedRow);
tupleValues.add(tupleRow);
}
// A LogicalValues tuple holds nothing but literals, and whether a value is one is a property of
// what it converts to rather than of the Substrait expression it came from: a struct converts
// to a ROW call and a list to an array constructor, literal or not. Neither belongs in a tuple
// anyway -- Calcite orders tuples by casting each value to Comparable, and the value of a row
// literal is a list of RexLiterals, which are not.
boolean encodableAsTuples =
tupleValues.stream().flatMap(List::stream).allMatch(value -> value instanceof RexLiteral);

For an R-by-C table that is R extra lists plus R times C extra slots held through the whole conversion, and unwrapNullabilityCast runs on every literal even for a table that ends up computed. Since encodableAsTuples needs the unwrapped form to decide, one list plus a running flag -- stop filling the tuple list at the first non-literal, unwrap in a second pass when the flag survives -- covers both branches.

Found while reviewing #1189.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions