From 52cd0948d32d393f33a5282a870bb100b5d68447 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Tue, 13 Oct 2026 14:41:55 +0200 Subject: [PATCH 1/3] feat(core)!: bump substrait to v0.105.0 Spec v0.105.0 lets a producer bound protobuf nesting by moving expression subtrees into `PlanRel.detached_expressions` / `ExtendedExpression.detached_expressions` and referring to them by `Expression.detached_expression_ordinal`. Protobuf parsers reject messages nested more than 100 levels below the root, so a deeply nested plan serialized by substrait-java could not be parsed back, even by substrait-java. Reading: `ProtoExpressionConverter` resolves an ordinal by converting its entry in place, so the entry binds to the referencing site's schema and scope. `DetachedExpressions` enforces the spec's rules on the reachable entries: in range, at most once, acyclic, and not itself an ordinal. `ProtoPlanConverter` and `ProtoExtendedExpressionConverter` supply each container's table; `ProtoRelConverter.from(Rel, List)` does the same for a single `PlanRel`. Writing: `ExpressionDetacher` runs on each `PlanRel` and `ExtendedExpression` after POJO -> proto. A container within protobuf's default limit of 100 levels is returned unchanged, so only plans that default parsers would reject change. Otherwise an expression that does not fit is detached once it lies deeper than half the limit. The detacher is on by default and can be tuned or disabled through new `PlanProtoConverter` / `ExtendedExpressionProtoConverter` constructors. `avg` over a decimal now returns `DECIMAL?<38,S>`. Isthmus already emitted a nullable average; a stale test comment is corrected. The top-level `description` that extension files may now carry is ignored, as the parser already did for unknown keys. Closes #1352 BREAKING CHANGE: `avg` over a decimal resolves to a nullable `DECIMAL?<38,S>`, and plans or extended expressions whose expressions nest deeper than 100 protobuf levels are now serialized with detached expression subtrees, which consumers older than spec v0.105.0 cannot read. --- .../expression/proto/DetachedExpressions.java | 107 +++++++ .../expression/proto/ExpressionDetacher.java | 267 +++++++++++++++++ .../proto/ProtoExpressionConverter.java | 32 ++ .../ExtendedExpressionProtoConverter.java | 23 +- .../ProtoExtendedExpressionConverter.java | 7 +- .../io/substrait/plan/PlanProtoConverter.java | 38 ++- .../io/substrait/plan/ProtoPlanConverter.java | 2 +- .../substrait/relation/ProtoRelConverter.java | 35 +++ .../DetachedExpressionRoundtripTest.java | 275 ++++++++++++++++++ gradle/libs.versions.toml | 2 +- .../isthmus/SubstraitTypeSystemTest.java | 4 +- 11 files changed, 780 insertions(+), 12 deletions(-) create mode 100644 core/src/main/java/io/substrait/expression/proto/DetachedExpressions.java create mode 100644 core/src/main/java/io/substrait/expression/proto/ExpressionDetacher.java create mode 100644 core/src/test/java/io/substrait/type/proto/DetachedExpressionRoundtripTest.java diff --git a/core/src/main/java/io/substrait/expression/proto/DetachedExpressions.java b/core/src/main/java/io/substrait/expression/proto/DetachedExpressions.java new file mode 100644 index 000000000..d2a85ae07 --- /dev/null +++ b/core/src/main/java/io/substrait/expression/proto/DetachedExpressions.java @@ -0,0 +1,107 @@ +package io.substrait.expression.proto; + +import io.substrait.proto.Expression.RexTypeCase; +import java.util.ArrayList; +import java.util.BitSet; +import java.util.Collections; +import java.util.List; +import java.util.Objects; +import java.util.function.Function; + +/** + * The {@code detached_expressions} of one {@code PlanRel} or {@code ExtendedExpression}, resolving + * the {@code Expression.detached_expression_ordinal} references in it (spec v0.105.0). + * + *

Detachment is an encoding of the expression tree, so an ordinal is resolved by converting its + * entry in place of the reference: the entry binds to the input schema and correlation scope of the + * referencing site. Resolution enforces the rules the spec places on the portion of the table + * reachable from the container: an ordinal is in range, refers to an entry that is not itself an + * ordinal, is referenced at most once, and is not part of a cycle. Entries that nothing references + * are neither converted nor validated. + * + *

An instance records which entries it has resolved, so it serves one conversion of one + * container. The converted model inlines every resolved entry and keeps no table, so an ordinal + * inside a {@code google.protobuf.Any} payload that a converter retains without interpreting loses + * its target. + */ +public final class DetachedExpressions { + private static final DetachedExpressions NONE = + new DetachedExpressions(Collections.emptyList(), false); + + private final List entries; + private final boolean inContainer; + private final BitSet referenced = new BitSet(); + private final BitSet resolving = new BitSet(); + + private DetachedExpressions(List entries, boolean inContainer) { + this.entries = entries; + this.inContainer = inContainer; + } + + /** + * Returns the resolver for expressions converted outside any {@code PlanRel} or {@code + * ExtendedExpression}, where every detached expression ordinal is invalid. + * + * @return a resolver that rejects every ordinal + */ + public static DetachedExpressions none() { + return NONE; + } + + /** + * Creates a resolver over the {@code detached_expressions} of one container. + * + * @param entries the container's detached expressions, indexed by ordinal + * @return a resolver for that container + */ + public static DetachedExpressions of(List entries) { + return new DetachedExpressions( + Collections.unmodifiableList(new ArrayList<>(Objects.requireNonNull(entries, "entries"))), + true); + } + + /** + * Converts the entry an ordinal refers to. + * + * @param ordinal the {@code detached_expression_ordinal}, an unsigned 32-bit value + * @param converter converts the entry, resolving any ordinals within it through this resolver + * @param the converted type + * @return the converted entry + * @throws IllegalArgumentException if the reference breaks one of the spec's rules + */ + public T resolve(int ordinal, Function converter) { + long index = Integer.toUnsignedLong(ordinal); + if (!inContainer) { + throw new IllegalArgumentException( + String.format( + "Detached expression ordinal %d is outside a PlanRel or ExtendedExpression", index)); + } + if (index >= entries.size()) { + throw new IllegalArgumentException( + String.format( + "Detached expression ordinal %d is out of range for %d detached expressions", + index, entries.size())); + } + int i = (int) index; + if (resolving.get(i)) { + throw new IllegalArgumentException( + String.format("Detached expression %d references itself through a cycle", i)); + } + if (referenced.get(i)) { + throw new IllegalArgumentException( + String.format("Detached expression %d is referenced more than once", i)); + } + io.substrait.proto.Expression entry = entries.get(i); + if (entry.getRexTypeCase() == RexTypeCase.DETACHED_EXPRESSION_ORDINAL) { + throw new IllegalArgumentException( + String.format("Detached expression %d is itself a detached expression ordinal", i)); + } + referenced.set(i); + resolving.set(i); + try { + return converter.apply(entry); + } finally { + resolving.clear(i); + } + } +} diff --git a/core/src/main/java/io/substrait/expression/proto/ExpressionDetacher.java b/core/src/main/java/io/substrait/expression/proto/ExpressionDetacher.java new file mode 100644 index 000000000..99cc402e4 --- /dev/null +++ b/core/src/main/java/io/substrait/expression/proto/ExpressionDetacher.java @@ -0,0 +1,267 @@ +package io.substrait.expression.proto; + +import com.google.protobuf.CodedInputStream; +import com.google.protobuf.Descriptors.Descriptor; +import com.google.protobuf.Descriptors.FieldDescriptor; +import com.google.protobuf.Message; +import com.google.protobuf.WireFormat; +import io.substrait.proto.Expression.RexTypeCase; +import io.substrait.proto.ExtendedExpression; +import io.substrait.proto.PlanRel; +import java.io.IOException; +import java.util.ArrayList; +import java.util.IdentityHashMap; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; +import java.util.stream.Collectors; + +/** + * Bounds the protobuf nesting depth of a {@code PlanRel} or {@code ExtendedExpression} by moving + * expression subtrees into its {@code detached_expressions} and referring to them by {@code + * Expression.detached_expression_ordinal} (spec v0.105.0). + * + *

Protobuf parsers limit how deeply messages may nest; protobuf-java, for one, rejects a message + * nested more than {@value #DEFAULT_MAX_DEPTH} levels below the root it parses. A container already + * within the limit is returned unchanged. Otherwise, an expression that does not fit at its + * position is detached once it lies deeper than half the limit, and its subtree is processed again + * from its new position directly below the container. Ordinals are assigned in the order the + * subtrees are detached, so a detached entry refers only to entries after it. + * + *

Detachment shortens expression nesting only. Nesting in a chain of relations, in a literal or + * type, or inside a {@code google.protobuf.Any} payload is left as is, so the result may still + * exceed the limit. + */ +public final class ExpressionDetacher { + /** The default parse recursion limit of protobuf-java, and of the C++ and upb runtimes. */ + public static final int DEFAULT_MAX_DEPTH = 100; + + private static final ExpressionDetacher DEFAULT = new ExpressionDetacher(DEFAULT_MAX_DEPTH); + private static final ExpressionDetacher DISABLED = new ExpressionDetacher(Integer.MAX_VALUE); + + /** Nesting depth of a {@code PlanRel} below the {@code Plan} it is parsed as part of. */ + private static final int PLAN_REL_DEPTH = 1; + + /** The message-typed fields of each message type, the only ones that add nesting. */ + private static final Map> MESSAGE_FIELDS = + new ConcurrentHashMap<>(); + + private final int maxDepth; + + private ExpressionDetacher(int maxDepth) { + this.maxDepth = maxDepth; + } + + /** + * Returns a detacher that keeps a plan within the default parse recursion limit of {@value + * #DEFAULT_MAX_DEPTH}. + * + * @return the default detacher + */ + public static ExpressionDetacher defaultDetacher() { + return DEFAULT; + } + + /** + * Returns a detacher that never detaches an expression. + * + * @return a detacher that leaves every container unchanged + */ + public static ExpressionDetacher disabled() { + return DISABLED; + } + + /** + * Returns a detacher for a consumer that parses up to {@code maxDepth} levels of nesting below + * the root message, the root itself being at depth 0. + * + * @param maxDepth the deepest nesting a consumer accepts + * @return a detacher for that limit + */ + public static ExpressionDetacher withMaxDepth(int maxDepth) { + if (maxDepth < 0) { + throw new IllegalArgumentException("maxDepth must not be negative: " + maxDepth); + } + return new ExpressionDetacher(maxDepth); + } + + /** + * Detaches expressions of a {@code PlanRel} that is parsed as part of a {@code Plan}. + * + * @param planRel the relation to bound + * @return {@code planRel} itself if it fits, otherwise a copy with detached expressions + */ + public PlanRel detach(PlanRel planRel) { + return new Pass(PLAN_REL_DEPTH, planRel.getDetachedExpressionsList()).run(planRel); + } + + /** + * Detaches expressions of an {@code ExtendedExpression} that is parsed as the root message. + * + * @param extendedExpression the extended expression to bound + * @return {@code extendedExpression} itself if it fits, otherwise a copy with detached + * expressions + */ + public ExtendedExpression detach(ExtendedExpression extendedExpression) { + return new Pass(0, extendedExpression.getDetachedExpressionsList()).run(extendedExpression); + } + + /** One detachment of one container. */ + private final class Pass { + private final int containerDepth; + private final int existingEntries; + private final List entries; + private final Map heights = new IdentityHashMap<>(); + + Pass(int containerDepth, List existing) { + this.containerDepth = containerDepth; + this.existingEntries = existing.size(); + this.entries = new ArrayList<>(existing); + } + + @SuppressWarnings("unchecked") + M run(M container) { + if (maxDepth == Integer.MAX_VALUE + || maxDepth >= containerDepth && within(container, maxDepth - containerDepth)) { + return container; + } + FieldDescriptor detachedField = + container.getDescriptorForType().findFieldByName("detached_expressions"); + Message.Builder builder = container.toBuilder().clearField(detachedField); + // An existing entry is not moved, so the ordinals referring to it keep their target. + for (int i = 0; i < existingEntries; i++) { + entries.set(i, (io.substrait.proto.Expression) children(entries.get(i), entryDepth())); + } + for (FieldDescriptor field : messageFields(container)) { + if (!field.equals(detachedField)) { + copyRewritten(container, builder, field, containerDepth + 1); + } + } + for (io.substrait.proto.Expression entry : entries) { + builder.addRepeatedField(detachedField, entry); + } + return (M) builder.build(); + } + + private int entryDepth() { + return containerDepth + 1; + } + + private boolean fits(Message message, int depth) { + return height(message) <= maxDepth - depth; + } + + /** Returns the message at {@code depth}, with whatever it contains detached as needed. */ + private Message rewrite(Message message, int depth) { + if (fits(message, depth)) { + return message; + } + if (message instanceof io.substrait.proto.Expression + && depth > Math.max(maxDepth / 2, entryDepth()) + && ((io.substrait.proto.Expression) message).getRexTypeCase() + != RexTypeCase.DETACHED_EXPRESSION_ORDINAL) { + int ordinal = entries.size(); + entries.add(null); + entries.set(ordinal, (io.substrait.proto.Expression) children(message, entryDepth())); + return io.substrait.proto.Expression.newBuilder() + .setDetachedExpressionOrdinal(ordinal) + .build(); + } + return children(message, depth); + } + + /** Returns the message at {@code depth} with each child message rewritten. */ + private Message children(Message message, int depth) { + if (fits(message, depth)) { + return message; + } + Message.Builder builder = message.toBuilder(); + for (FieldDescriptor field : messageFields(message)) { + copyRewritten(message, builder, field, depth + 1); + } + return builder.build(); + } + + private void copyRewritten( + Message message, Message.Builder builder, FieldDescriptor field, int childDepth) { + if (field.isRepeated()) { + for (int i = 0; i < message.getRepeatedFieldCount(field); i++) { + builder.setRepeatedField( + field, i, rewrite((Message) message.getRepeatedField(field, i), childDepth)); + } + } else if (message.hasField(field)) { + builder.setField(field, rewrite((Message) message.getField(field), childDepth)); + } + } + + /** Returns how many levels of messages are nested below {@code message}. */ + private int height(Message message) { + Integer known = heights.get(message); + if (known != null) { + return known; + } + int height = 0; + for (FieldDescriptor field : messageFields(message)) { + if (field.isRepeated()) { + for (int i = 0; i < message.getRepeatedFieldCount(field); i++) { + height = Math.max(height, 1 + height((Message) message.getRepeatedField(field, i))); + } + } else if (message.hasField(field)) { + height = Math.max(height, 1 + height((Message) message.getField(field))); + } + } + heights.put(message, height); + return height; + } + } + + /** + * Returns whether at most {@code levels} levels of messages are nested below {@code message}. + * + *

Every container goes through this check, so it scans the serialized form, which the + * generated code writes without reflection, rather than walking the message tree. + */ + private static boolean within(Message message, int levels) { + try { + return within( + message.getDescriptorForType(), + CodedInputStream.newInstance(message.toByteArray()), + levels); + } catch (IOException e) { + throw new IllegalStateException( + "Failed to scan a serialized " + message.getDescriptorForType().getFullName(), e); + } + } + + private static boolean within(Descriptor type, CodedInputStream input, int levels) + throws IOException { + while (!input.isAtEnd()) { + int tag = input.readTag(); + if (WireFormat.getTagWireType(tag) == WireFormat.WIRETYPE_LENGTH_DELIMITED) { + FieldDescriptor field = type.findFieldByNumber(WireFormat.getTagFieldNumber(tag)); + if (field != null && field.getJavaType() == FieldDescriptor.JavaType.MESSAGE) { + if (levels == 0) { + return false; + } + int limit = input.pushLimit(input.readRawVarint32()); + if (!within(field.getMessageType(), input, levels - 1)) { + return false; + } + input.popLimit(limit); + continue; + } + } + input.skipField(tag); + } + return true; + } + + private static List messageFields(Message message) { + return MESSAGE_FIELDS.computeIfAbsent( + message.getDescriptorForType(), + descriptor -> + descriptor.getFields().stream() + .filter(field -> field.getJavaType() == FieldDescriptor.JavaType.MESSAGE) + .collect(Collectors.toList())); + } +} diff --git a/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java b/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java index 847ab34b8..079f8dc14 100644 --- a/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java +++ b/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java @@ -40,10 +40,14 @@ public class ProtoExpressionConverter { private final ProtoTypeConverter protoTypeConverter; private final ProtoRelConverter protoRelConverter; private final LambdaBuilder lambdaBuilder = new LambdaBuilder(); + private final DetachedExpressions detachedExpressions; /** * Creates a converter resolving functions and types against the given extensions. * + *

Detached expression ordinals resolve against {@link ProtoRelConverter#detachedExpressions()} + * of {@code relConverter}, and are rejected when it is {@code null}. + * * @param lookup used to resolve function and type references * @param extensions the extension collection providing definitions * @param rootType the root struct type that field references are resolved against @@ -54,11 +58,37 @@ public ProtoExpressionConverter( SimpleExtension.ExtensionCollection extensions, Type.Struct rootType, ProtoRelConverter relConverter) { + this( + lookup, + extensions, + rootType, + relConverter, + relConverter == null ? DetachedExpressions.none() : relConverter.detachedExpressions()); + } + + /** + * Creates a converter resolving functions and types against the given extensions, and detached + * expression ordinals against the given container. + * + * @param lookup used to resolve function and type references + * @param extensions the extension collection providing definitions + * @param rootType the root struct type that field references are resolved against + * @param relConverter converter for nested relations (e.g. subqueries) + * @param detachedExpressions the detached expressions of the containing {@code PlanRel} or {@code + * ExtendedExpression} + */ + public ProtoExpressionConverter( + ExtensionLookup lookup, + SimpleExtension.ExtensionCollection extensions, + Type.Struct rootType, + ProtoRelConverter relConverter, + DetachedExpressions detachedExpressions) { this.lookup = lookup; this.extensions = extensions; this.rootType = Objects.requireNonNull(rootType, "rootType"); this.protoTypeConverter = new ProtoTypeConverter(lookup, extensions); this.protoRelConverter = relConverter; + this.detachedExpressions = Objects.requireNonNull(detachedExpressions, "detachedExpressions"); } /** @@ -364,6 +394,8 @@ public Type visit(Type.Struct type) throws RuntimeException { + ecv.getExecutionContextVariableTypeCase()); } } + case DETACHED_EXPRESSION_ORDINAL: + return detachedExpressions.resolve(expr.getDetachedExpressionOrdinal(), this::from); default: throw new IllegalArgumentException("Unknown type: " + expr.getRexTypeCase()); } diff --git a/core/src/main/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverter.java b/core/src/main/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverter.java index 45d6f8e68..07913ca56 100644 --- a/core/src/main/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverter.java +++ b/core/src/main/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverter.java @@ -1,5 +1,6 @@ package io.substrait.extendedexpression; +import io.substrait.expression.proto.ExpressionDetacher; import io.substrait.expression.proto.ExpressionProtoConverter; import io.substrait.extension.ExtensionCollector; import io.substrait.proto.ExpressionReference; @@ -7,12 +8,32 @@ import io.substrait.relation.AggregateFunctionProtoConverter; import io.substrait.type.proto.TypeProtoConverter; import io.substrait.util.EmptyVisitationContext; +import java.util.Objects; /** * Converts from {@link io.substrait.extendedexpression.ExtendedExpression} to {@link * io.substrait.proto.ExtendedExpression} */ public class ExtendedExpressionProtoConverter { + private final ExpressionDetacher expressionDetacher; + + /** + * Creates a converter that uses {@link ExpressionDetacher#defaultDetacher()}, which leaves an + * extended expression unchanged unless its expressions nest too deeply for protobuf parsers with + * default limits. + */ + public ExtendedExpressionProtoConverter() { + this(ExpressionDetacher.defaultDetacher()); + } + + /** + * Creates a converter that bounds the nesting depth of its output with the given detacher. + * + * @param expressionDetacher bounds the nesting depth of each {@link ExtendedExpression} + */ + public ExtendedExpressionProtoConverter(ExpressionDetacher expressionDetacher) { + this.expressionDetacher = Objects.requireNonNull(expressionDetacher, "expressionDetacher"); + } /** * Converts an {@link io.substrait.extendedexpression.ExtendedExpression} into its protobuf @@ -71,6 +92,6 @@ public ExtendedExpression toProto( if (extendedExpression.getAdvancedExtension().isPresent()) { builder.setAdvancedExtensions(extendedExpression.getAdvancedExtension().get()); } - return builder.build(); + return expressionDetacher.detach(builder.build()); } } diff --git a/core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java b/core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java index 55ec5e6b0..a67d18f15 100644 --- a/core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java +++ b/core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java @@ -1,6 +1,7 @@ package io.substrait.extendedexpression; import io.substrait.expression.Expression; +import io.substrait.expression.proto.DetachedExpressions; import io.substrait.expression.proto.ProtoExpressionConverter; import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.ExtensionCollector; @@ -61,7 +62,11 @@ public ExtendedExpression from(io.substrait.proto.ExtendedExpression extendedExp ProtoExpressionConverter protoExpressionConverter = new ProtoExpressionConverter( - functionLookup, this.extensionCollection, namedStruct.struct(), null); + functionLookup, + this.extensionCollection, + namedStruct.struct(), + null, + DetachedExpressions.of(extendedExpression.getDetachedExpressionsList())); List expressionReferences = new ArrayList<>(); diff --git a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java index 61e9ff9b3..01b46abc5 100644 --- a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java +++ b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java @@ -1,5 +1,6 @@ package io.substrait.plan; +import io.substrait.expression.proto.ExpressionDetacher; import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.ExtensionCollector; import io.substrait.extension.ExtensionProtoConverter; @@ -21,6 +22,8 @@ public class PlanProtoConverter { @NonNull private final ExtensionCollection extensionCollection; + @NonNull private final ExpressionDetacher expressionDetacher; + /** Default constructor. */ public PlanProtoConverter() { this(DefaultExtensionCatalog.DEFAULT_COLLECTION); @@ -53,6 +56,27 @@ public PlanProtoConverter(@NonNull final ExtensionProtoConverter extension public PlanProtoConverter( @NonNull final ExtensionCollection extensionCollection, @NonNull final ExtensionProtoConverter extensionProtoConverter) { + this(extensionCollection, extensionProtoConverter, ExpressionDetacher.defaultDetacher()); + } + + /** + * Constructor with custom {@link ExtensionCollection}, {@link ExtensionProtoConverter} and {@link + * ExpressionDetacher}. + * + *

The other constructors use {@link ExpressionDetacher#defaultDetacher()}, which leaves a plan + * unchanged unless its expressions nest too deeply for protobuf parsers with default limits. + * + * @param extensionCollection custom {@link ExtensionCollection} to use, must not be null + * @param extensionProtoConverter custom {@link ExtensionProtoConverter} to use, must not be null + * @param expressionDetacher bounds the nesting depth of each {@link PlanRel}, must not be null + */ + public PlanProtoConverter( + @NonNull final ExtensionCollection extensionCollection, + @NonNull final ExtensionProtoConverter extensionProtoConverter, + @NonNull final ExpressionDetacher expressionDetacher) { + if (expressionDetacher == null) { + throw new IllegalArgumentException("ExpressionDetacher is required"); + } if (extensionCollection == null) { throw new IllegalArgumentException("ExtensionCollection is required"); } @@ -61,6 +85,7 @@ public PlanProtoConverter( } this.extensionCollection = extensionCollection; this.extensionProtoConverter = extensionProtoConverter; + this.expressionDetacher = expressionDetacher; } /** @@ -78,12 +103,13 @@ public Plan toProto(final io.substrait.plan.Plan plan) { new RelProtoConverter(functionCollector, extensionProtoConverter) .toProto(root.getInput()); planRels.add( - PlanRel.newBuilder() - .setRoot( - io.substrait.proto.RelRoot.newBuilder() - .setInput(input) - .addAllNames(root.getNames())) - .build()); + expressionDetacher.detach( + PlanRel.newBuilder() + .setRoot( + io.substrait.proto.RelRoot.newBuilder() + .setInput(input) + .addAllNames(root.getNames())) + .build())); } final Plan.Builder builder = Plan.newBuilder() diff --git a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java index 629665f2e..ed6bcfcdd 100644 --- a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java +++ b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java @@ -96,7 +96,7 @@ public Plan from(io.substrait.proto.Plan plan) { List roots = new ArrayList<>(); for (PlanRel planRel : plan.getRelationsList()) { io.substrait.proto.RelRoot root = planRel.getRoot(); - Rel rel = relConverter.from(root.getInput()); + Rel rel = relConverter.from(root.getInput(), planRel.getDetachedExpressionsList()); roots.add(Plan.Root.builder().input(rel).names(root.getNamesList()).build()); } diff --git a/core/src/main/java/io/substrait/relation/ProtoRelConverter.java b/core/src/main/java/io/substrait/relation/ProtoRelConverter.java index 83b3d9e38..c1592086e 100644 --- a/core/src/main/java/io/substrait/relation/ProtoRelConverter.java +++ b/core/src/main/java/io/substrait/relation/ProtoRelConverter.java @@ -3,6 +3,7 @@ import io.substrait.expression.Expression; import io.substrait.expression.FieldReference; import io.substrait.expression.MaskExpression; +import io.substrait.expression.proto.DetachedExpressions; import io.substrait.expression.proto.ProtoExpressionConverter; import io.substrait.expression.proto.ProtoMaskExpressionConverter; import io.substrait.extension.AdvancedExtension; @@ -102,6 +103,9 @@ public class ProtoRelConverter { */ private final Map anchorScopes = new HashMap<>(); + /** The detached expressions of the {@code PlanRel} being converted, if any. */ + private DetachedExpressions detachedExpressions = DetachedExpressions.none(); + /** * Constructor with custom {@link ExtensionLookup}. * @@ -188,6 +192,37 @@ public Rel from(io.substrait.proto.Rel rel) { return converted; } + /** + * Converts a relation of a {@code PlanRel}, resolving the detached expression ordinals in it + * against that {@code PlanRel}'s {@code detached_expressions}. + * + * @param rel the protobuf relation to convert + * @param detachedExpressions the {@code detached_expressions} of the containing {@code PlanRel} + * @return the converted result + */ + public Rel from( + io.substrait.proto.Rel rel, List detachedExpressions) { + DetachedExpressions enclosing = this.detachedExpressions; + this.detachedExpressions = DetachedExpressions.of(detachedExpressions); + try { + return from(rel); + } finally { + this.detachedExpressions = enclosing; + } + } + + /** + * Returns the detached expressions that expressions converted for the current relation resolve + * their ordinals against: those of the {@code PlanRel} passed to {@link + * #from(io.substrait.proto.Rel, List)}, or a resolver that rejects every ordinal outside such a + * conversion. + * + * @return the current detached expressions + */ + public DetachedExpressions detachedExpressions() { + return detachedExpressions; + } + private Rel dispatch(io.substrait.proto.Rel rel) { io.substrait.proto.Rel.RelTypeCase relType = rel.getRelTypeCase(); switch (relType) { diff --git a/core/src/test/java/io/substrait/type/proto/DetachedExpressionRoundtripTest.java b/core/src/test/java/io/substrait/type/proto/DetachedExpressionRoundtripTest.java new file mode 100644 index 000000000..f909c42b6 --- /dev/null +++ b/core/src/test/java/io/substrait/type/proto/DetachedExpressionRoundtripTest.java @@ -0,0 +1,275 @@ +package io.substrait.type.proto; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.google.protobuf.Descriptors.FieldDescriptor; +import com.google.protobuf.InvalidProtocolBufferException; +import com.google.protobuf.Message; +import io.substrait.expression.Expression; +import io.substrait.expression.ExpressionCreator; +import io.substrait.expression.proto.ExpressionDetacher; +import io.substrait.extendedexpression.ExtendedExpression; +import io.substrait.extendedexpression.ExtendedExpressionProtoConverter; +import io.substrait.extendedexpression.ImmutableExpressionReference; +import io.substrait.extendedexpression.ImmutableExtendedExpression; +import io.substrait.extendedexpression.ProtoExtendedExpressionConverter; +import io.substrait.extension.DefaultExtensionCatalog; +import io.substrait.extension.ExtensionProtoConverter; +import io.substrait.plan.Plan; +import io.substrait.plan.PlanProtoConverter; +import io.substrait.plan.ProtoPlanConverter; +import io.substrait.proto.PlanRel; +import io.substrait.relation.NamedScan; +import io.substrait.relation.Rel; +import io.substrait.type.NamedStruct; +import io.substrait.utils.RelSamples; +import io.substrait.utils.StringHolderHandlingProtoRelConverter; +import io.substrait.utils.StringHolderHandlingRelProtoConverter; +import java.util.List; +import java.util.Map; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +/** + * Verifies detached expression subtrees (spec v0.105.0): the writer moves expressions into {@code + * detached_expressions} only when they nest too deeply, the reader resolves the ordinals back into + * the same expressions, and the reader enforces the rules the spec places on the references. + */ +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +class DetachedExpressionRoundtripTest extends StringHolderRoundtripTestBase { + + /** A detacher with no depth to spare, so it detaches every expression it can. */ + static final ExpressionDetacher DETACH_ALL = ExpressionDetacher.withMaxDepth(0); + + /** Deep enough that the nested additions exceed protobuf-java's parse recursion limit. */ + static final int DEEP = 200; + + final Map, Rel> relSamples = new RelSamples(sb, extensions).samples(); + + final NamedScan scan = sb.namedScan(List.of("t"), List.of("a"), List.of(R.I32)); + + Stream samples() { + return relSamples.entrySet().stream() + .map(sample -> Arguments.of(sample.getKey().getSimpleName(), sample.getValue())); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("samples") + void everyRelationRoundtripsWithEveryExpressionDetached(String relationType, Rel rel) { + io.substrait.proto.Rel protoRel = + new StringHolderHandlingRelProtoConverter(functionCollector).toProto(rel); + PlanRel detached = + DETACH_ALL.detach( + PlanRel.newBuilder() + .setRoot(io.substrait.proto.RelRoot.newBuilder().setInput(protoRel)) + .build()); + + if (containsExpression(protoRel)) { + assertTrue(detached.getDetachedExpressionsCount() > 0, "no expression was detached"); + assertNotEquals(protoRel, detached.getRoot().getInput()); + } + Rel returned = + new StringHolderHandlingProtoRelConverter(functionCollector, extensions) + .from(detached.getRoot().getInput(), detached.getDetachedExpressionsList()); + assertEquals(rel, returned); + } + + @Test + void aShallowPlanIsLeftUnchanged() { + Plan plan = sb.plan(sb.root(sb.project(input -> List.of(nestedAdditions(input, 3)), scan))); + io.substrait.proto.Plan proto = new PlanProtoConverter().toProto(plan); + assertEquals(0, proto.getRelations(0).getDetachedExpressionsCount()); + assertEquals(proto, planConverter(ExpressionDetacher.disabled()).toProto(plan)); + } + + @Test + void aDeepPlanIsDetachedToParseWithDefaultLimits() throws InvalidProtocolBufferException { + Plan plan = sb.plan(sb.root(sb.project(input -> List.of(nestedAdditions(input, DEEP)), scan))); + + byte[] undetached = planConverter(ExpressionDetacher.disabled()).toProto(plan).toByteArray(); + assertThrows( + InvalidProtocolBufferException.class, () -> io.substrait.proto.Plan.parseFrom(undetached)); + + io.substrait.proto.Plan proto = + io.substrait.proto.Plan.parseFrom(new PlanProtoConverter().toProto(plan).toByteArray()); + assertTrue(proto.getRelations(0).getDetachedExpressionsCount() > 0); + assertTrue(depth(proto) <= ExpressionDetacher.DEFAULT_MAX_DEPTH, () -> "depth " + depth(proto)); + assertEquals(plan, new ProtoPlanConverter().from(proto)); + } + + @Test + void aDeepExtendedExpressionIsDetachedToParseWithDefaultLimits() + throws InvalidProtocolBufferException { + ExtendedExpression extendedExpression = + ImmutableExtendedExpression.builder() + .baseSchema(NamedStruct.of(List.of("a"), R.struct(R.I32))) + .addReferredExpressions( + ImmutableExpressionReference.builder() + .expression(nestedAdditions(scan, DEEP)) + .addOutputNames("deep") + .build()) + .build(); + + io.substrait.proto.ExtendedExpression proto = + io.substrait.proto.ExtendedExpression.parseFrom( + new ExtendedExpressionProtoConverter().toProto(extendedExpression).toByteArray()); + assertTrue(proto.getDetachedExpressionsCount() > 0); + assertTrue(depth(proto) <= ExpressionDetacher.DEFAULT_MAX_DEPTH, () -> "depth " + depth(proto)); + assertEquals(extendedExpression, new ProtoExtendedExpressionConverter().from(proto)); + + io.substrait.proto.ExtendedExpression all = + new ExtendedExpressionProtoConverter(DETACH_ALL).toProto(extendedExpression); + assertEquals(extendedExpression, new ProtoExtendedExpressionConverter().from(all)); + } + + @Test + void aContainerWithinTheLimitIsReturnedAsIs() { + PlanRel planRel = + PlanRel.newBuilder() + .setRoot( + io.substrait.proto.RelRoot.newBuilder() + .setInput(relProtoConverter.toProto(filterOn(sb.bool(true))))) + .build(); + assertSame(planRel, ExpressionDetacher.defaultDetacher().detach(planRel)); + } + + @Test + void anUnreferencedEntryIsNeitherConvertedNorValidated() { + PlanRel planRel = + DETACH_ALL.detach(planRel(filterOn(sb.equal(sb.fieldReference(scan, 0), sb.i32(1))))); + PlanRel withUnreachable = + planRel.toBuilder() + .addDetachedExpressions(ordinal(0)) + .addDetachedExpressions(io.substrait.proto.Expression.getDefaultInstance()) + .build(); + assertEquals( + protoRelConverter.from(planRel.getRoot().getInput(), planRel.getDetachedExpressionsList()), + protoRelConverter.from( + withUnreachable.getRoot().getInput(), withUnreachable.getDetachedExpressionsList())); + } + + @Test + void anOrdinalOutOfRangeIsRejected() { + assertRejected("out of range", condition(ordinal(1)), List.of(literal())); + assertRejected("out of range", condition(ordinal(0)), List.of()); + } + + @Test + void anEntryReferencedTwiceIsRejected() { + assertRejected( + "referenced more than once", condition(equal(ordinal(0), ordinal(0))), List.of(literal())); + } + + @Test + void aCycleIsRejected() { + assertRejected( + "cycle", + condition(ordinal(0)), + List.of(equal(ordinal(1), literal()), equal(ordinal(0), literal()))); + } + + @Test + void anEntryThatIsItselfAnOrdinalIsRejected() { + assertRejected( + "itself a detached expression ordinal", + condition(ordinal(0)), + List.of(ordinal(1), literal())); + } + + @Test + void anOrdinalOutsideAContainerIsRejected() { + IllegalArgumentException error = + assertThrows( + IllegalArgumentException.class, () -> protoRelConverter.from(condition(ordinal(0)))); + assertTrue(error.getMessage().contains("outside a PlanRel"), error.getMessage()); + } + + private void assertRejected( + String message, + io.substrait.proto.Rel rel, + List detachedExpressions) { + IllegalArgumentException error = + assertThrows( + IllegalArgumentException.class, () -> protoRelConverter.from(rel, detachedExpressions)); + assertTrue(error.getMessage().contains(message), error.getMessage()); + } + + private Rel filterOn(Expression condition) { + return sb.filter(input -> condition, scan); + } + + private PlanRel planRel(Rel rel) { + return PlanRel.newBuilder() + .setRoot(io.substrait.proto.RelRoot.newBuilder().setInput(relProtoConverter.toProto(rel))) + .build(); + } + + /** A filter over {@link #scan} with the given proto condition. */ + private io.substrait.proto.Rel condition(io.substrait.proto.Expression condition) { + io.substrait.proto.Rel rel = relProtoConverter.toProto(filterOn(sb.bool(true))); + return rel.toBuilder().setFilter(rel.getFilter().toBuilder().setCondition(condition)).build(); + } + + /** An {@code equal:any_any} of the two proto operands, shaped as the writer emits it. */ + private io.substrait.proto.Expression equal( + io.substrait.proto.Expression left, io.substrait.proto.Expression right) { + io.substrait.proto.Expression template = + expressionProtoConverter.toProto(sb.equal(sb.i32(1), sb.i32(1))); + io.substrait.proto.Expression.ScalarFunction.Builder function = + template.getScalarFunction().toBuilder(); + function.getArgumentsBuilder(0).setValue(left); + function.getArgumentsBuilder(1).setValue(right); + return template.toBuilder().setScalarFunction(function).build(); + } + + private io.substrait.proto.Expression literal() { + return expressionProtoConverter.toProto(sb.i32(1)); + } + + private static io.substrait.proto.Expression ordinal(int ordinal) { + return io.substrait.proto.Expression.newBuilder().setDetachedExpressionOrdinal(ordinal).build(); + } + + private Expression nestedAdditions(Rel input, int depth) { + Expression expression = sb.fieldReference(input, 0); + for (int i = 0; i < depth; i++) { + expression = sb.add(expression, ExpressionCreator.i32(false, i)); + } + return expression; + } + + private static PlanProtoConverter planConverter(ExpressionDetacher detacher) { + return new PlanProtoConverter( + DefaultExtensionCatalog.DEFAULT_COLLECTION, new ExtensionProtoConverter<>(), detacher); + } + + private static boolean containsExpression(Message message) { + if (message instanceof io.substrait.proto.Expression) { + return true; + } + return children(message).anyMatch(DetachedExpressionRoundtripTest::containsExpression); + } + + /** Returns how many levels of messages nest below {@code message}, as a parser counts them. */ + private static int depth(Message message) { + return children(message).mapToInt(child -> 1 + depth(child)).max().orElse(0); + } + + private static Stream children(Message message) { + return message.getAllFields().entrySet().stream() + .filter(field -> field.getKey().getJavaType() == FieldDescriptor.JavaType.MESSAGE) + .flatMap( + field -> + field.getKey().isRepeated() + ? ((List) field.getValue()).stream().map(Message.class::cast) + : Stream.of((Message) field.getValue())); + } +} diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 3c0fc4d57..a5d671457 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -31,7 +31,7 @@ spark-3-5 = "3.5.4" spark-4-0 = "4.0.2" spotless = "8.10.3" # substrait-packaging artifacts, versioned by the Substrait spec release they are generated from. -substrait-packaging = "0.104.0" +substrait-packaging = "0.105.0" testcontainers = "2.0.5" validator = "3.0.8" diff --git a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java index 98649e283..932a8006f 100644 --- a/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java +++ b/isthmus/src/test/java/io/substrait/isthmus/SubstraitTypeSystemTest.java @@ -119,8 +119,8 @@ void sumAndAvgDeriveWhatTheExtensionsDeclare() { /** * The same in a query: isthmus's own SUM and AVG take their types from the type system, where - * they used to repeat the argument's. The average stays nullable, the right answer for an empty - * group, although avg:dec declares a required result. + * they used to repeat the argument's. The average is nullable, the right answer for an empty + * group, as avg:dec declares. */ @Test void sumAndAvgInAQueryTakeTheTypeSystemsTypes() throws Exception { From 1388df6e8f8796f987220c523476f90e4a5d71aa Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Tue, 13 Oct 2026 15:44:22 +0200 Subject: [PATCH 2/3] refactor(core): shape detached-subtree API for detached relations substrait-io/substrait#1236 adds `detached_rels` to the same containers, so name and shape the new API for both tables before it ships: `NestingDetacher` and `DetachedSubtrees`, built from the container rather than from its expression list, and `ProtoRelConverter.from(PlanRel)` in place of `from(Rel, List)`. The latter also serves callers that convert a plan relation by relation, which `from(RelRoot)` cannot for a plan with detached expressions, and refuses a bare-relation PlanRel with an accurate message. Adds round trips for detached entries inside lambda bodies, correlated subqueries (steps_out and rel_reference) and IN predicates. --- ...Expressions.java => DetachedSubtrees.java} | 50 +++--- ...sionDetacher.java => NestingDetacher.java} | 16 +- .../proto/ProtoExpressionConverter.java | 17 +- .../ExtendedExpressionProtoConverter.java | 16 +- .../ProtoExtendedExpressionConverter.java | 4 +- .../io/substrait/plan/PlanProtoConverter.java | 22 +-- .../io/substrait/plan/ProtoPlanConverter.java | 5 +- .../substrait/relation/ProtoRelConverter.java | 43 ++--- .../DetachedExpressionRoundtripTest.java | 163 ++++++++++++++++-- 9 files changed, 241 insertions(+), 95 deletions(-) rename core/src/main/java/io/substrait/expression/proto/{DetachedExpressions.java => DetachedSubtrees.java} (65%) rename core/src/main/java/io/substrait/expression/proto/{ExpressionDetacher.java => NestingDetacher.java} (95%) diff --git a/core/src/main/java/io/substrait/expression/proto/DetachedExpressions.java b/core/src/main/java/io/substrait/expression/proto/DetachedSubtrees.java similarity index 65% rename from core/src/main/java/io/substrait/expression/proto/DetachedExpressions.java rename to core/src/main/java/io/substrait/expression/proto/DetachedSubtrees.java index d2a85ae07..6a478a469 100644 --- a/core/src/main/java/io/substrait/expression/proto/DetachedExpressions.java +++ b/core/src/main/java/io/substrait/expression/proto/DetachedSubtrees.java @@ -1,16 +1,17 @@ package io.substrait.expression.proto; import io.substrait.proto.Expression.RexTypeCase; -import java.util.ArrayList; +import io.substrait.proto.ExtendedExpression; +import io.substrait.proto.PlanRel; import java.util.BitSet; import java.util.Collections; import java.util.List; -import java.util.Objects; import java.util.function.Function; /** - * The {@code detached_expressions} of one {@code PlanRel} or {@code ExtendedExpression}, resolving - * the {@code Expression.detached_expression_ordinal} references in it (spec v0.105.0). + * The detached subtrees of one {@code PlanRel} or {@code ExtendedExpression}, resolving the {@code + * Expression.detached_expression_ordinal} references in it against its {@code detached_expressions} + * (spec v0.105.0). * *

Detachment is an encoding of the expression tree, so an ordinal is resolved by converting its * entry in place of the reference: the entry binds to the input schema and correlation scope of the @@ -20,20 +21,20 @@ * are neither converted nor validated. * *

An instance records which entries it has resolved, so it serves one conversion of one - * container. The converted model inlines every resolved entry and keeps no table, so an ordinal - * inside a {@code google.protobuf.Any} payload that a converter retains without interpreting loses - * its target. + * container. The converted model inlines every resolved entry and keeps no table. The spec forbids + * a consumer that retains an opaque {@code google.protobuf.Any} payload from dropping the entries + * that payload's ordinals refer to, so a converter must not retain such a payload uninterpreted + * when it holds ordinals. */ -public final class DetachedExpressions { - private static final DetachedExpressions NONE = - new DetachedExpressions(Collections.emptyList(), false); +public final class DetachedSubtrees { + private static final DetachedSubtrees NONE = new DetachedSubtrees(Collections.emptyList(), false); private final List entries; private final boolean inContainer; private final BitSet referenced = new BitSet(); private final BitSet resolving = new BitSet(); - private DetachedExpressions(List entries, boolean inContainer) { + private DetachedSubtrees(List entries, boolean inContainer) { this.entries = entries; this.inContainer = inContainer; } @@ -44,24 +45,32 @@ private DetachedExpressions(List entries, boolean * * @return a resolver that rejects every ordinal */ - public static DetachedExpressions none() { + public static DetachedSubtrees none() { return NONE; } /** - * Creates a resolver over the {@code detached_expressions} of one container. + * Creates a resolver over the detached subtrees of a {@code PlanRel}. * - * @param entries the container's detached expressions, indexed by ordinal + * @param planRel the container whose ordinals are resolved * @return a resolver for that container */ - public static DetachedExpressions of(List entries) { - return new DetachedExpressions( - Collections.unmodifiableList(new ArrayList<>(Objects.requireNonNull(entries, "entries"))), - true); + public static DetachedSubtrees of(PlanRel planRel) { + return new DetachedSubtrees(planRel.getDetachedExpressionsList(), true); } /** - * Converts the entry an ordinal refers to. + * Creates a resolver over the detached subtrees of an {@code ExtendedExpression}. + * + * @param extendedExpression the container whose ordinals are resolved + * @return a resolver for that container + */ + public static DetachedSubtrees of(ExtendedExpression extendedExpression) { + return new DetachedSubtrees(extendedExpression.getDetachedExpressionsList(), true); + } + + /** + * Converts the {@code detached_expressions} entry an ordinal refers to. * * @param ordinal the {@code detached_expression_ordinal}, an unsigned 32-bit value * @param converter converts the entry, resolving any ordinals within it through this resolver @@ -69,7 +78,8 @@ public static DetachedExpressions of(List entries * @return the converted entry * @throws IllegalArgumentException if the reference breaks one of the spec's rules */ - public T resolve(int ordinal, Function converter) { + public T resolveExpression( + int ordinal, Function converter) { long index = Integer.toUnsignedLong(ordinal); if (!inContainer) { throw new IllegalArgumentException( diff --git a/core/src/main/java/io/substrait/expression/proto/ExpressionDetacher.java b/core/src/main/java/io/substrait/expression/proto/NestingDetacher.java similarity index 95% rename from core/src/main/java/io/substrait/expression/proto/ExpressionDetacher.java rename to core/src/main/java/io/substrait/expression/proto/NestingDetacher.java index 99cc402e4..05e2bfe75 100644 --- a/core/src/main/java/io/substrait/expression/proto/ExpressionDetacher.java +++ b/core/src/main/java/io/substrait/expression/proto/NestingDetacher.java @@ -32,12 +32,12 @@ * type, or inside a {@code google.protobuf.Any} payload is left as is, so the result may still * exceed the limit. */ -public final class ExpressionDetacher { +public final class NestingDetacher { /** The default parse recursion limit of protobuf-java, and of the C++ and upb runtimes. */ public static final int DEFAULT_MAX_DEPTH = 100; - private static final ExpressionDetacher DEFAULT = new ExpressionDetacher(DEFAULT_MAX_DEPTH); - private static final ExpressionDetacher DISABLED = new ExpressionDetacher(Integer.MAX_VALUE); + private static final NestingDetacher DEFAULT = new NestingDetacher(DEFAULT_MAX_DEPTH); + private static final NestingDetacher DISABLED = new NestingDetacher(Integer.MAX_VALUE); /** Nesting depth of a {@code PlanRel} below the {@code Plan} it is parsed as part of. */ private static final int PLAN_REL_DEPTH = 1; @@ -48,7 +48,7 @@ public final class ExpressionDetacher { private final int maxDepth; - private ExpressionDetacher(int maxDepth) { + private NestingDetacher(int maxDepth) { this.maxDepth = maxDepth; } @@ -58,7 +58,7 @@ private ExpressionDetacher(int maxDepth) { * * @return the default detacher */ - public static ExpressionDetacher defaultDetacher() { + public static NestingDetacher defaultDetacher() { return DEFAULT; } @@ -67,7 +67,7 @@ public static ExpressionDetacher defaultDetacher() { * * @return a detacher that leaves every container unchanged */ - public static ExpressionDetacher disabled() { + public static NestingDetacher disabled() { return DISABLED; } @@ -78,11 +78,11 @@ public static ExpressionDetacher disabled() { * @param maxDepth the deepest nesting a consumer accepts * @return a detacher for that limit */ - public static ExpressionDetacher withMaxDepth(int maxDepth) { + public static NestingDetacher withMaxDepth(int maxDepth) { if (maxDepth < 0) { throw new IllegalArgumentException("maxDepth must not be negative: " + maxDepth); } - return new ExpressionDetacher(maxDepth); + return new NestingDetacher(maxDepth); } /** diff --git a/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java b/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java index 079f8dc14..717fcff99 100644 --- a/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java +++ b/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java @@ -40,13 +40,14 @@ public class ProtoExpressionConverter { private final ProtoTypeConverter protoTypeConverter; private final ProtoRelConverter protoRelConverter; private final LambdaBuilder lambdaBuilder = new LambdaBuilder(); - private final DetachedExpressions detachedExpressions; + private final DetachedSubtrees detachedSubtrees; /** * Creates a converter resolving functions and types against the given extensions. * - *

Detached expression ordinals resolve against {@link ProtoRelConverter#detachedExpressions()} - * of {@code relConverter}, and are rejected when it is {@code null}. + *

Detached ordinals resolve against the {@link ProtoRelConverter#detachedSubtrees()} that + * {@code relConverter} holds when this converter is constructed, and are rejected when it is + * {@code null}. * * @param lookup used to resolve function and type references * @param extensions the extension collection providing definitions @@ -63,7 +64,7 @@ public ProtoExpressionConverter( extensions, rootType, relConverter, - relConverter == null ? DetachedExpressions.none() : relConverter.detachedExpressions()); + relConverter == null ? DetachedSubtrees.none() : relConverter.detachedSubtrees()); } /** @@ -74,7 +75,7 @@ public ProtoExpressionConverter( * @param extensions the extension collection providing definitions * @param rootType the root struct type that field references are resolved against * @param relConverter converter for nested relations (e.g. subqueries) - * @param detachedExpressions the detached expressions of the containing {@code PlanRel} or {@code + * @param detachedSubtrees the detached subtrees of the containing {@code PlanRel} or {@code * ExtendedExpression} */ public ProtoExpressionConverter( @@ -82,13 +83,13 @@ public ProtoExpressionConverter( SimpleExtension.ExtensionCollection extensions, Type.Struct rootType, ProtoRelConverter relConverter, - DetachedExpressions detachedExpressions) { + DetachedSubtrees detachedSubtrees) { this.lookup = lookup; this.extensions = extensions; this.rootType = Objects.requireNonNull(rootType, "rootType"); this.protoTypeConverter = new ProtoTypeConverter(lookup, extensions); this.protoRelConverter = relConverter; - this.detachedExpressions = Objects.requireNonNull(detachedExpressions, "detachedExpressions"); + this.detachedSubtrees = Objects.requireNonNull(detachedSubtrees, "detachedSubtrees"); } /** @@ -395,7 +396,7 @@ public Type visit(Type.Struct type) throws RuntimeException { } } case DETACHED_EXPRESSION_ORDINAL: - return detachedExpressions.resolve(expr.getDetachedExpressionOrdinal(), this::from); + return detachedSubtrees.resolveExpression(expr.getDetachedExpressionOrdinal(), this::from); default: throw new IllegalArgumentException("Unknown type: " + expr.getRexTypeCase()); } diff --git a/core/src/main/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverter.java b/core/src/main/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverter.java index 07913ca56..7f88bfce6 100644 --- a/core/src/main/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverter.java +++ b/core/src/main/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverter.java @@ -1,7 +1,7 @@ package io.substrait.extendedexpression; -import io.substrait.expression.proto.ExpressionDetacher; import io.substrait.expression.proto.ExpressionProtoConverter; +import io.substrait.expression.proto.NestingDetacher; import io.substrait.extension.ExtensionCollector; import io.substrait.proto.ExpressionReference; import io.substrait.proto.ExtendedExpression; @@ -15,24 +15,24 @@ * io.substrait.proto.ExtendedExpression} */ public class ExtendedExpressionProtoConverter { - private final ExpressionDetacher expressionDetacher; + private final NestingDetacher nestingDetacher; /** - * Creates a converter that uses {@link ExpressionDetacher#defaultDetacher()}, which leaves an + * Creates a converter that uses {@link NestingDetacher#defaultDetacher()}, which leaves an * extended expression unchanged unless its expressions nest too deeply for protobuf parsers with * default limits. */ public ExtendedExpressionProtoConverter() { - this(ExpressionDetacher.defaultDetacher()); + this(NestingDetacher.defaultDetacher()); } /** * Creates a converter that bounds the nesting depth of its output with the given detacher. * - * @param expressionDetacher bounds the nesting depth of each {@link ExtendedExpression} + * @param nestingDetacher bounds the nesting depth of each {@link ExtendedExpression} */ - public ExtendedExpressionProtoConverter(ExpressionDetacher expressionDetacher) { - this.expressionDetacher = Objects.requireNonNull(expressionDetacher, "expressionDetacher"); + public ExtendedExpressionProtoConverter(NestingDetacher nestingDetacher) { + this.nestingDetacher = Objects.requireNonNull(nestingDetacher, "nestingDetacher"); } /** @@ -92,6 +92,6 @@ public ExtendedExpression toProto( if (extendedExpression.getAdvancedExtension().isPresent()) { builder.setAdvancedExtensions(extendedExpression.getAdvancedExtension().get()); } - return expressionDetacher.detach(builder.build()); + return nestingDetacher.detach(builder.build()); } } diff --git a/core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java b/core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java index a67d18f15..d37cf8006 100644 --- a/core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java +++ b/core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java @@ -1,7 +1,7 @@ package io.substrait.extendedexpression; import io.substrait.expression.Expression; -import io.substrait.expression.proto.DetachedExpressions; +import io.substrait.expression.proto.DetachedSubtrees; import io.substrait.expression.proto.ProtoExpressionConverter; import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.ExtensionCollector; @@ -66,7 +66,7 @@ public ExtendedExpression from(io.substrait.proto.ExtendedExpression extendedExp this.extensionCollection, namedStruct.struct(), null, - DetachedExpressions.of(extendedExpression.getDetachedExpressionsList())); + DetachedSubtrees.of(extendedExpression)); List expressionReferences = new ArrayList<>(); diff --git a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java index 01b46abc5..5a1df2cc7 100644 --- a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java +++ b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java @@ -1,6 +1,6 @@ package io.substrait.plan; -import io.substrait.expression.proto.ExpressionDetacher; +import io.substrait.expression.proto.NestingDetacher; import io.substrait.extension.DefaultExtensionCatalog; import io.substrait.extension.ExtensionCollector; import io.substrait.extension.ExtensionProtoConverter; @@ -22,7 +22,7 @@ public class PlanProtoConverter { @NonNull private final ExtensionCollection extensionCollection; - @NonNull private final ExpressionDetacher expressionDetacher; + @NonNull private final NestingDetacher nestingDetacher; /** Default constructor. */ public PlanProtoConverter() { @@ -56,26 +56,26 @@ public PlanProtoConverter(@NonNull final ExtensionProtoConverter extension public PlanProtoConverter( @NonNull final ExtensionCollection extensionCollection, @NonNull final ExtensionProtoConverter extensionProtoConverter) { - this(extensionCollection, extensionProtoConverter, ExpressionDetacher.defaultDetacher()); + this(extensionCollection, extensionProtoConverter, NestingDetacher.defaultDetacher()); } /** * Constructor with custom {@link ExtensionCollection}, {@link ExtensionProtoConverter} and {@link - * ExpressionDetacher}. + * NestingDetacher}. * - *

The other constructors use {@link ExpressionDetacher#defaultDetacher()}, which leaves a plan + *

The other constructors use {@link NestingDetacher#defaultDetacher()}, which leaves a plan * unchanged unless its expressions nest too deeply for protobuf parsers with default limits. * * @param extensionCollection custom {@link ExtensionCollection} to use, must not be null * @param extensionProtoConverter custom {@link ExtensionProtoConverter} to use, must not be null - * @param expressionDetacher bounds the nesting depth of each {@link PlanRel}, must not be null + * @param nestingDetacher bounds the nesting depth of each {@link PlanRel}, must not be null */ public PlanProtoConverter( @NonNull final ExtensionCollection extensionCollection, @NonNull final ExtensionProtoConverter extensionProtoConverter, - @NonNull final ExpressionDetacher expressionDetacher) { - if (expressionDetacher == null) { - throw new IllegalArgumentException("ExpressionDetacher is required"); + @NonNull final NestingDetacher nestingDetacher) { + if (nestingDetacher == null) { + throw new IllegalArgumentException("NestingDetacher is required"); } if (extensionCollection == null) { throw new IllegalArgumentException("ExtensionCollection is required"); @@ -85,7 +85,7 @@ public PlanProtoConverter( } this.extensionCollection = extensionCollection; this.extensionProtoConverter = extensionProtoConverter; - this.expressionDetacher = expressionDetacher; + this.nestingDetacher = nestingDetacher; } /** @@ -103,7 +103,7 @@ public Plan toProto(final io.substrait.plan.Plan plan) { new RelProtoConverter(functionCollector, extensionProtoConverter) .toProto(root.getInput()); planRels.add( - expressionDetacher.detach( + nestingDetacher.detach( PlanRel.newBuilder() .setRoot( io.substrait.proto.RelRoot.newBuilder() diff --git a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java index ed6bcfcdd..a39ba9d65 100644 --- a/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java +++ b/core/src/main/java/io/substrait/plan/ProtoPlanConverter.java @@ -9,7 +9,6 @@ import io.substrait.plan.Plan.ExecutionBehavior.VariableEvaluationMode; import io.substrait.proto.PlanRel; import io.substrait.relation.ProtoRelConverter; -import io.substrait.relation.Rel; import java.util.ArrayList; import java.util.List; import java.util.Optional; @@ -95,9 +94,7 @@ public Plan from(io.substrait.proto.Plan plan) { ProtoRelConverter relConverter = getProtoRelConverter(functionLookup); List roots = new ArrayList<>(); for (PlanRel planRel : plan.getRelationsList()) { - io.substrait.proto.RelRoot root = planRel.getRoot(); - Rel rel = relConverter.from(root.getInput(), planRel.getDetachedExpressionsList()); - roots.add(Plan.Root.builder().input(rel).names(root.getNamesList()).build()); + roots.add(relConverter.from(planRel)); } ImmutableVersion.Builder versionBuilder = diff --git a/core/src/main/java/io/substrait/relation/ProtoRelConverter.java b/core/src/main/java/io/substrait/relation/ProtoRelConverter.java index c1592086e..a2fee09a0 100644 --- a/core/src/main/java/io/substrait/relation/ProtoRelConverter.java +++ b/core/src/main/java/io/substrait/relation/ProtoRelConverter.java @@ -3,7 +3,7 @@ import io.substrait.expression.Expression; import io.substrait.expression.FieldReference; import io.substrait.expression.MaskExpression; -import io.substrait.expression.proto.DetachedExpressions; +import io.substrait.expression.proto.DetachedSubtrees; import io.substrait.expression.proto.ProtoExpressionConverter; import io.substrait.expression.proto.ProtoMaskExpressionConverter; import io.substrait.extension.AdvancedExtension; @@ -103,8 +103,8 @@ public class ProtoRelConverter { */ private final Map anchorScopes = new HashMap<>(); - /** The detached expressions of the {@code PlanRel} being converted, if any. */ - private DetachedExpressions detachedExpressions = DetachedExpressions.none(); + /** The detached subtrees of the {@code PlanRel} being converted, if any. */ + private DetachedSubtrees detachedSubtrees = DetachedSubtrees.none(); /** * Constructor with custom {@link ExtensionLookup}. @@ -193,34 +193,37 @@ public Rel from(io.substrait.proto.Rel rel) { } /** - * Converts a relation of a {@code PlanRel}, resolving the detached expression ordinals in it - * against that {@code PlanRel}'s {@code detached_expressions}. + * Converts the root of a {@code PlanRel}, resolving the detached ordinals in it against that + * {@code PlanRel}'s detached subtrees. * - * @param rel the protobuf relation to convert - * @param detachedExpressions the {@code detached_expressions} of the containing {@code PlanRel} - * @return the converted result + * @param planRel the protobuf {@code PlanRel} to convert, which must hold a root + * @return the converted root + * @throws IllegalArgumentException if {@code planRel} holds a bare relation rather than a root */ - public Rel from( - io.substrait.proto.Rel rel, List detachedExpressions) { - DetachedExpressions enclosing = this.detachedExpressions; - this.detachedExpressions = DetachedExpressions.of(detachedExpressions); + public Plan.Root from(io.substrait.proto.PlanRel planRel) { + if (!planRel.hasRoot()) { + throw new IllegalArgumentException( + "Only PlanRel roots are supported, got " + planRel.getRelTypeCase()); + } + DetachedSubtrees enclosing = this.detachedSubtrees; + this.detachedSubtrees = DetachedSubtrees.of(planRel); try { - return from(rel); + return from(planRel.getRoot()); } finally { - this.detachedExpressions = enclosing; + this.detachedSubtrees = enclosing; } } /** - * Returns the detached expressions that expressions converted for the current relation resolve - * their ordinals against: those of the {@code PlanRel} passed to {@link - * #from(io.substrait.proto.Rel, List)}, or a resolver that rejects every ordinal outside such a + * Returns the detached subtrees that expressions converted for the current relation resolve their + * ordinals against: those of the {@code PlanRel} passed to {@link + * #from(io.substrait.proto.PlanRel)}, or a resolver that rejects every ordinal outside such a * conversion. * - * @return the current detached expressions + * @return the current detached subtrees */ - public DetachedExpressions detachedExpressions() { - return detachedExpressions; + public DetachedSubtrees detachedSubtrees() { + return detachedSubtrees; } private Rel dispatch(io.substrait.proto.Rel rel) { diff --git a/core/src/test/java/io/substrait/type/proto/DetachedExpressionRoundtripTest.java b/core/src/test/java/io/substrait/type/proto/DetachedExpressionRoundtripTest.java index f909c42b6..7d6b26f62 100644 --- a/core/src/test/java/io/substrait/type/proto/DetachedExpressionRoundtripTest.java +++ b/core/src/test/java/io/substrait/type/proto/DetachedExpressionRoundtripTest.java @@ -11,7 +11,9 @@ import com.google.protobuf.Message; import io.substrait.expression.Expression; import io.substrait.expression.ExpressionCreator; -import io.substrait.expression.proto.ExpressionDetacher; +import io.substrait.expression.FieldReference; +import io.substrait.expression.LambdaBuilder; +import io.substrait.expression.proto.NestingDetacher; import io.substrait.extendedexpression.ExtendedExpression; import io.substrait.extendedexpression.ExtendedExpressionProtoConverter; import io.substrait.extendedexpression.ImmutableExpressionReference; @@ -25,6 +27,7 @@ import io.substrait.proto.PlanRel; import io.substrait.relation.NamedScan; import io.substrait.relation.Rel; +import io.substrait.relation.Rel.Remap; import io.substrait.type.NamedStruct; import io.substrait.utils.RelSamples; import io.substrait.utils.StringHolderHandlingProtoRelConverter; @@ -47,7 +50,7 @@ class DetachedExpressionRoundtripTest extends StringHolderRoundtripTestBase { /** A detacher with no depth to spare, so it detaches every expression it can. */ - static final ExpressionDetacher DETACH_ALL = ExpressionDetacher.withMaxDepth(0); + static final NestingDetacher DETACH_ALL = NestingDetacher.withMaxDepth(0); /** Deep enough that the nested additions exceed protobuf-java's parse recursion limit. */ static final int DEEP = 200; @@ -56,6 +59,12 @@ class DetachedExpressionRoundtripTest extends StringHolderRoundtripTestBase { final NamedScan scan = sb.namedScan(List.of("t"), List.of("a"), List.of(R.I32)); + final Rel orders = + sb.namedScan(List.of("orders"), List.of("o_orderkey", "o_custkey"), List.of(R.I64, R.I64)); + + final Rel customer = + sb.namedScan(List.of("customer"), List.of("c_custkey", "c_nationkey"), List.of(R.I64, R.I64)); + Stream samples() { return relSamples.entrySet().stream() .map(sample -> Arguments.of(sample.getKey().getSimpleName(), sample.getValue())); @@ -78,7 +87,8 @@ void everyRelationRoundtripsWithEveryExpressionDetached(String relationType, Rel } Rel returned = new StringHolderHandlingProtoRelConverter(functionCollector, extensions) - .from(detached.getRoot().getInput(), detached.getDetachedExpressionsList()); + .from(detached) + .getInput(); assertEquals(rel, returned); } @@ -87,21 +97,21 @@ void aShallowPlanIsLeftUnchanged() { Plan plan = sb.plan(sb.root(sb.project(input -> List.of(nestedAdditions(input, 3)), scan))); io.substrait.proto.Plan proto = new PlanProtoConverter().toProto(plan); assertEquals(0, proto.getRelations(0).getDetachedExpressionsCount()); - assertEquals(proto, planConverter(ExpressionDetacher.disabled()).toProto(plan)); + assertEquals(proto, planConverter(NestingDetacher.disabled()).toProto(plan)); } @Test void aDeepPlanIsDetachedToParseWithDefaultLimits() throws InvalidProtocolBufferException { Plan plan = sb.plan(sb.root(sb.project(input -> List.of(nestedAdditions(input, DEEP)), scan))); - byte[] undetached = planConverter(ExpressionDetacher.disabled()).toProto(plan).toByteArray(); + byte[] undetached = planConverter(NestingDetacher.disabled()).toProto(plan).toByteArray(); assertThrows( InvalidProtocolBufferException.class, () -> io.substrait.proto.Plan.parseFrom(undetached)); io.substrait.proto.Plan proto = io.substrait.proto.Plan.parseFrom(new PlanProtoConverter().toProto(plan).toByteArray()); assertTrue(proto.getRelations(0).getDetachedExpressionsCount() > 0); - assertTrue(depth(proto) <= ExpressionDetacher.DEFAULT_MAX_DEPTH, () -> "depth " + depth(proto)); + assertTrue(depth(proto) <= NestingDetacher.DEFAULT_MAX_DEPTH, () -> "depth " + depth(proto)); assertEquals(plan, new ProtoPlanConverter().from(proto)); } @@ -122,7 +132,7 @@ void aDeepExtendedExpressionIsDetachedToParseWithDefaultLimits() io.substrait.proto.ExtendedExpression.parseFrom( new ExtendedExpressionProtoConverter().toProto(extendedExpression).toByteArray()); assertTrue(proto.getDetachedExpressionsCount() > 0); - assertTrue(depth(proto) <= ExpressionDetacher.DEFAULT_MAX_DEPTH, () -> "depth " + depth(proto)); + assertTrue(depth(proto) <= NestingDetacher.DEFAULT_MAX_DEPTH, () -> "depth " + depth(proto)); assertEquals(extendedExpression, new ProtoExtendedExpressionConverter().from(proto)); io.substrait.proto.ExtendedExpression all = @@ -138,7 +148,7 @@ void aContainerWithinTheLimitIsReturnedAsIs() { io.substrait.proto.RelRoot.newBuilder() .setInput(relProtoConverter.toProto(filterOn(sb.bool(true))))) .build(); - assertSame(planRel, ExpressionDetacher.defaultDetacher().detach(planRel)); + assertSame(planRel, NestingDetacher.defaultDetacher().detach(planRel)); } @Test @@ -150,10 +160,7 @@ void anUnreferencedEntryIsNeitherConvertedNorValidated() { .addDetachedExpressions(ordinal(0)) .addDetachedExpressions(io.substrait.proto.Expression.getDefaultInstance()) .build(); - assertEquals( - protoRelConverter.from(planRel.getRoot().getInput(), planRel.getDetachedExpressionsList()), - protoRelConverter.from( - withUnreachable.getRoot().getInput(), withUnreachable.getDetachedExpressionsList())); + assertEquals(protoRelConverter.from(planRel), protoRelConverter.from(withUnreachable)); } @Test @@ -192,13 +199,141 @@ void anOrdinalOutsideAContainerIsRejected() { assertTrue(error.getMessage().contains("outside a PlanRel"), error.getMessage()); } + @Test + void aPlanRelWithoutARootIsRejected() { + IllegalArgumentException error = + assertThrows( + IllegalArgumentException.class, + () -> + protoRelConverter.from( + PlanRel.newBuilder().setRel(relProtoConverter.toProto(scan)).build())); + assertTrue(error.getMessage().contains("Only PlanRel roots"), error.getMessage()); + } + + @Test + void aLambdaBodyResolvesInTheLambdaScope() { + LambdaBuilder lb = new LambdaBuilder(); + Rel input = sb.namedScan(List.of("t"), List.of("xs", "k"), List.of(R.list(R.I64), R.I64)); + assertDetachedRoundTrip( + sb.project( + in -> + List.of( + sb.scalarFn( + DefaultExtensionCatalog.FUNCTIONS_LIST, + "transform:list_func", + R.list(R.I64), + sb.fieldReference(in, 0), + lb.lambda( + List.of(R.I64), + p -> sb.add(sb.add(p.ref(0), sb.fieldReference(in, 1)), sb.i64(1))))), + Remap.of(List.of(2)), + input)); + } + + @Test + void aNestedLambdaBodyResolvesInItsOwnScope() { + LambdaBuilder lb = new LambdaBuilder(); + Rel input = sb.namedScan(List.of("t"), List.of("xs"), List.of(R.list(R.list(R.I64)))); + assertDetachedRoundTrip( + sb.project( + in -> + List.of( + sb.scalarFn( + DefaultExtensionCatalog.FUNCTIONS_LIST, + "transform:list_func", + R.list(R.list(R.I64)), + sb.fieldReference(in, 0), + lb.lambda( + List.of(R.list(R.I64)), + outer -> + sb.scalarFn( + DefaultExtensionCatalog.FUNCTIONS_LIST, + "transform:list_func", + R.list(R.I64), + outer.ref(0), + lb.lambda( + List.of(R.I64), + inner -> sb.add(inner.ref(0), sb.i64(1))))))), + Remap.of(List.of(1)), + input)); + } + + @Test + void aStepsOutReferenceResolvesAgainstTheEnclosingQuery() { + Rel subquery = + correlated(FieldReference.newRootStructOuterReference(1, orders.getRecordType(), 1)); + assertDetachedRoundTrip( + sb.project( + input -> List.of(sb.fieldReference(input, 0), sb.scalarSubquery(subquery, R.I64)), + Remap.of(List.of(2, 3)), + orders)); + } + + @Test + void aRelReferenceResolvesAgainstTheAnchoredRelation() { + Rel anchored = orders.withRelAnchor(1); + Rel subquery = + correlated( + FieldReference.newRootStructOuterReferenceByRelReference( + 1, anchored.getRecordType(), 1)); + assertDetachedRoundTrip( + sb.project( + input -> List.of(sb.fieldReference(input, 0), sb.scalarSubquery(subquery, R.I64)), + Remap.of(List.of(2, 3)), + anchored)); + } + + @Test + void inPredicateNeedlesAndACorrelatedHaystackResolveInTheirScopes() { + Rel haystack = + sb.project( + input -> List.of(sb.fieldReference(input, 0)), + Remap.of(List.of(2)), + sb.filter( + input -> + sb.equal( + sb.fieldReference(input, 1), + FieldReference.newRootStructOuterReference(1, orders.getRecordType(), 1)), + customer)); + assertDetachedRoundTrip( + sb.filter( + input -> + sb.inPredicate( + haystack, + sb.add(sb.fieldReference(input, 0), sb.i64(1)), + sb.fieldReference(input, 1)), + orders)); + } + + /** Round-trips {@code rel} as a plan with every expression detached. */ + private void assertDetachedRoundTrip(Rel rel) { + Plan plan = sb.plan(sb.root(rel)); + io.substrait.proto.Plan proto = planConverter(DETACH_ALL).toProto(plan); + assertTrue(proto.getRelations(0).getDetachedExpressionsCount() > 0); + assertEquals(plan, new ProtoPlanConverter().from(proto)); + } + + /** A subquery over customer whose filter compares c_custkey with the given outer reference. */ + private Rel correlated(FieldReference outerReference) { + return sb.project( + input -> List.of(sb.fieldReference(input, 1)), + Remap.of(List.of(2)), + sb.filter(input -> sb.equal(sb.fieldReference(input, 0), outerReference), customer)); + } + private void assertRejected( String message, io.substrait.proto.Rel rel, List detachedExpressions) { IllegalArgumentException error = assertThrows( - IllegalArgumentException.class, () -> protoRelConverter.from(rel, detachedExpressions)); + IllegalArgumentException.class, + () -> + protoRelConverter.from( + PlanRel.newBuilder() + .setRoot(io.substrait.proto.RelRoot.newBuilder().setInput(rel)) + .addAllDetachedExpressions(detachedExpressions) + .build())); assertTrue(error.getMessage().contains(message), error.getMessage()); } @@ -246,7 +381,7 @@ private Expression nestedAdditions(Rel input, int depth) { return expression; } - private static PlanProtoConverter planConverter(ExpressionDetacher detacher) { + private static PlanProtoConverter planConverter(NestingDetacher detacher) { return new PlanProtoConverter( DefaultExtensionCatalog.DEFAULT_COLLECTION, new ExtensionProtoConverter<>(), detacher); } From 1e6de12e61f6508893b2c4526a86c5afb93263e1 Mon Sep 17 00:00:00 2001 From: Niels Pardon Date: Fri, 16 Oct 2026 10:14:04 +0200 Subject: [PATCH 3/3] fix(core): address detached-subtree review - Keep an extended expression's detached entries that its raw advanced extension may refer to: `ExtendedExpression.getRetainedDetachedExpressions` holds every entry up to the last unresolved one, and the writer puts them back at their ordinals before detaching. - Retry detachment with every expression below the container eligible when the first attempt leaves it too deep (a shallow subquery over a long relation chain), and return the container unchanged when neither attempt makes it fit. - Raise the declared plan version to at least 0.105.0, dropping the git hash, when a PlanRel is rewritten. - Resolve ordinals against the rel converter's current table when they are reached, not when the expression converter is built. - Add `ProtoRelConverter.fromRel(PlanRel)` for a bare-relation PlanRel. - Add `NestingDetacher.everyExpression()`, which detaches whatever it can, for exercising consumers. - Route the Spark `toProtoSubstrait` PlanRel through the detacher. --- .../expression/proto/DetachedSubtrees.java | 16 ++ .../expression/proto/NestingDetacher.java | 73 ++++++--- .../proto/ProtoExpressionConverter.java | 30 ++-- .../ExtendedExpression.java | 13 ++ .../ExtendedExpressionProtoConverter.java | 11 +- .../ProtoExtendedExpressionConverter.java | 11 +- .../io/substrait/plan/PlanProtoConverter.java | 46 +++++- .../substrait/relation/ProtoRelConverter.java | 43 +++++- .../DetachedExpressionRoundtripTest.java | 144 +++++++++++++++++- .../spark/logical/ToSubstraitRel.scala | 13 +- 10 files changed, 343 insertions(+), 57 deletions(-) diff --git a/core/src/main/java/io/substrait/expression/proto/DetachedSubtrees.java b/core/src/main/java/io/substrait/expression/proto/DetachedSubtrees.java index 6a478a469..00e6c3585 100644 --- a/core/src/main/java/io/substrait/expression/proto/DetachedSubtrees.java +++ b/core/src/main/java/io/substrait/expression/proto/DetachedSubtrees.java @@ -69,6 +69,22 @@ public static DetachedSubtrees of(ExtendedExpression extendedExpression) { return new DetachedSubtrees(extendedExpression.getDetachedExpressionsList(), true); } + /** + * Returns the {@code detached_expressions} entries a retained opaque payload may still refer to: + * every entry up to the last one this resolver has not resolved. A resolved entry is referenced + * only from the tree that inlined it, so dropping the trailing run of resolved entries removes no + * payload's target and renumbers nothing. + * + * @return the leading entries to retain, indexed by ordinal + */ + public List unresolvedPrefix() { + int end = entries.size(); + while (end > 0 && referenced.get(end - 1)) { + end--; + } + return entries.subList(0, end); + } + /** * Converts the {@code detached_expressions} entry an ordinal refers to. * diff --git a/core/src/main/java/io/substrait/expression/proto/NestingDetacher.java b/core/src/main/java/io/substrait/expression/proto/NestingDetacher.java index 05e2bfe75..39a2c1958 100644 --- a/core/src/main/java/io/substrait/expression/proto/NestingDetacher.java +++ b/core/src/main/java/io/substrait/expression/proto/NestingDetacher.java @@ -25,19 +25,23 @@ * nested more than {@value #DEFAULT_MAX_DEPTH} levels below the root it parses. A container already * within the limit is returned unchanged. Otherwise, an expression that does not fit at its * position is detached once it lies deeper than half the limit, and its subtree is processed again - * from its new position directly below the container. Ordinals are assigned in the order the - * subtrees are detached, so a detached entry refers only to entries after it. + * from its new position directly below the container. If that leaves the container too deep, for + * instance because the deep part is a chain of relations below a shallow subquery, detachment is + * retried with every expression below the container eligible. Ordinals are assigned in the order + * the subtrees are detached, so a detached entry refers only to entries after it. * - *

Detachment shortens expression nesting only. Nesting in a chain of relations, in a literal or - * type, or inside a {@code google.protobuf.Any} payload is left as is, so the result may still - * exceed the limit. + *

Detachment shortens expression nesting only, not nesting within a chain of relations, a + * literal or type, or a {@code google.protobuf.Any} payload. A container that still exceeds the + * limit after both attempts is returned unchanged, so that it does not need a consumer that reads + * detached expressions without gaining one that parses it. */ public final class NestingDetacher { /** The default parse recursion limit of protobuf-java, and of the C++ and upb runtimes. */ public static final int DEFAULT_MAX_DEPTH = 100; - private static final NestingDetacher DEFAULT = new NestingDetacher(DEFAULT_MAX_DEPTH); - private static final NestingDetacher DISABLED = new NestingDetacher(Integer.MAX_VALUE); + private static final NestingDetacher DEFAULT = new NestingDetacher(DEFAULT_MAX_DEPTH, false); + private static final NestingDetacher DISABLED = new NestingDetacher(Integer.MAX_VALUE, false); + private static final NestingDetacher EVERY_EXPRESSION = new NestingDetacher(0, true); /** Nesting depth of a {@code PlanRel} below the {@code Plan} it is parsed as part of. */ private static final int PLAN_REL_DEPTH = 1; @@ -47,9 +51,11 @@ public final class NestingDetacher { new ConcurrentHashMap<>(); private final int maxDepth; + private final boolean everyExpression; - private NestingDetacher(int maxDepth) { + private NestingDetacher(int maxDepth, boolean everyExpression) { this.maxDepth = maxDepth; + this.everyExpression = everyExpression; } /** @@ -71,6 +77,16 @@ public static NestingDetacher disabled() { return DISABLED; } + /** + * Returns a detacher that detaches every expression below the container, however shallow, so that + * a consumer's handling of detached expressions can be exercised on any plan. + * + * @return a detacher that detaches every expression it can + */ + public static NestingDetacher everyExpression() { + return EVERY_EXPRESSION; + } + /** * Returns a detacher for a consumer that parses up to {@code maxDepth} levels of nesting below * the root message, the root itself being at depth 0. @@ -82,7 +98,7 @@ public static NestingDetacher withMaxDepth(int maxDepth) { if (maxDepth < 0) { throw new IllegalArgumentException("maxDepth must not be negative: " + maxDepth); } - return new NestingDetacher(maxDepth); + return new NestingDetacher(maxDepth, false); } /** @@ -92,7 +108,7 @@ public static NestingDetacher withMaxDepth(int maxDepth) { * @return {@code planRel} itself if it fits, otherwise a copy with detached expressions */ public PlanRel detach(PlanRel planRel) { - return new Pass(PLAN_REL_DEPTH, planRel.getDetachedExpressionsList()).run(planRel); + return bound(planRel, PLAN_REL_DEPTH, planRel.getDetachedExpressionsList()); } /** @@ -103,28 +119,51 @@ public PlanRel detach(PlanRel planRel) { * expressions */ public ExtendedExpression detach(ExtendedExpression extendedExpression) { - return new Pass(0, extendedExpression.getDetachedExpressionsList()).run(extendedExpression); + return bound(extendedExpression, 0, extendedExpression.getDetachedExpressionsList()); + } + + private M bound( + M container, int containerDepth, List existing) { + int entryDepth = containerDepth + 1; + if (everyExpression) { + return new Pass(containerDepth, entryDepth, existing).run(container); + } + if (maxDepth == Integer.MAX_VALUE || fitsWhole(container, containerDepth)) { + return container; + } + for (int threshold : new int[] {Math.max(maxDepth / 2, entryDepth), entryDepth}) { + M detached = new Pass(containerDepth, threshold, existing).run(container); + if (fitsWhole(detached, containerDepth)) { + return detached; + } + } + return container; + } + + private boolean fitsWhole(Message container, int containerDepth) { + return maxDepth >= containerDepth && within(container, maxDepth - containerDepth); } /** One detachment of one container. */ private final class Pass { private final int containerDepth; + private final int threshold; private final int existingEntries; private final List entries; private final Map heights = new IdentityHashMap<>(); - Pass(int containerDepth, List existing) { + /** + * @param threshold the depth an expression must lie below to be detached + */ + Pass(int containerDepth, int threshold, List existing) { this.containerDepth = containerDepth; + this.threshold = threshold; this.existingEntries = existing.size(); this.entries = new ArrayList<>(existing); } @SuppressWarnings("unchecked") M run(M container) { - if (maxDepth == Integer.MAX_VALUE - || maxDepth >= containerDepth && within(container, maxDepth - containerDepth)) { - return container; - } FieldDescriptor detachedField = container.getDescriptorForType().findFieldByName("detached_expressions"); Message.Builder builder = container.toBuilder().clearField(detachedField); @@ -157,7 +196,7 @@ private Message rewrite(Message message, int depth) { return message; } if (message instanceof io.substrait.proto.Expression - && depth > Math.max(maxDepth / 2, entryDepth()) + && depth > threshold && ((io.substrait.proto.Expression) message).getRexTypeCase() != RexTypeCase.DETACHED_EXPRESSION_ORDINAL) { int ordinal = entries.size(); diff --git a/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java b/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java index 717fcff99..ce4d1f589 100644 --- a/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java +++ b/core/src/main/java/io/substrait/expression/proto/ProtoExpressionConverter.java @@ -40,14 +40,16 @@ public class ProtoExpressionConverter { private final ProtoTypeConverter protoTypeConverter; private final ProtoRelConverter protoRelConverter; private final LambdaBuilder lambdaBuilder = new LambdaBuilder(); + + /** The container's detached subtrees, or {@code null} to ask {@link #protoRelConverter}. */ private final DetachedSubtrees detachedSubtrees; /** * Creates a converter resolving functions and types against the given extensions. * *

Detached ordinals resolve against the {@link ProtoRelConverter#detachedSubtrees()} that - * {@code relConverter} holds when this converter is constructed, and are rejected when it is - * {@code null}. + * {@code relConverter} holds when an ordinal is reached, and are rejected when it is {@code + * null}. * * @param lookup used to resolve function and type references * @param extensions the extension collection providing definitions @@ -59,12 +61,7 @@ public ProtoExpressionConverter( SimpleExtension.ExtensionCollection extensions, Type.Struct rootType, ProtoRelConverter relConverter) { - this( - lookup, - extensions, - rootType, - relConverter, - relConverter == null ? DetachedSubtrees.none() : relConverter.detachedSubtrees()); + this(lookup, extensions, rootType, relConverter, null); } /** @@ -76,7 +73,8 @@ public ProtoExpressionConverter( * @param rootType the root struct type that field references are resolved against * @param relConverter converter for nested relations (e.g. subqueries) * @param detachedSubtrees the detached subtrees of the containing {@code PlanRel} or {@code - * ExtendedExpression} + * ExtendedExpression}, or {@code null} to resolve against those {@code relConverter} holds + * when an ordinal is reached */ public ProtoExpressionConverter( ExtensionLookup lookup, @@ -89,7 +87,16 @@ public ProtoExpressionConverter( this.rootType = Objects.requireNonNull(rootType, "rootType"); this.protoTypeConverter = new ProtoTypeConverter(lookup, extensions); this.protoRelConverter = relConverter; - this.detachedSubtrees = Objects.requireNonNull(detachedSubtrees, "detachedSubtrees"); + this.detachedSubtrees = detachedSubtrees; + } + + private DetachedSubtrees detachedSubtrees() { + if (detachedSubtrees != null) { + return detachedSubtrees; + } + return protoRelConverter == null + ? DetachedSubtrees.none() + : protoRelConverter.detachedSubtrees(); } /** @@ -396,7 +403,8 @@ public Type visit(Type.Struct type) throws RuntimeException { } } case DETACHED_EXPRESSION_ORDINAL: - return detachedSubtrees.resolveExpression(expr.getDetachedExpressionOrdinal(), this::from); + return detachedSubtrees() + .resolveExpression(expr.getDetachedExpressionOrdinal(), this::from); default: throw new IllegalArgumentException("Unknown type: " + expr.getRexTypeCase()); } diff --git a/core/src/main/java/io/substrait/extendedexpression/ExtendedExpression.java b/core/src/main/java/io/substrait/extendedexpression/ExtendedExpression.java index 3ea90c6df..daa777514 100644 --- a/core/src/main/java/io/substrait/extendedexpression/ExtendedExpression.java +++ b/core/src/main/java/io/substrait/extendedexpression/ExtendedExpression.java @@ -44,6 +44,19 @@ public abstract class ExtendedExpression { */ public abstract Optional getAdvancedExtension(); + /** + * Returns the {@code detached_expressions} entries that ordinals inside the {@link + * #getAdvancedExtension() advanced extension} may still refer to, at their original ordinals. + * + *

The advanced extension is kept as the raw proto, so an ordinal inside it is not resolved; + * the spec requires its target to keep its ordinal. Reading an extended expression that has an + * advanced extension retains every entry up to the last one no referred expression resolved, and + * writing one places them back at the same ordinals. Empty otherwise. + * + * @return the retained detached expressions, indexed by ordinal + */ + public abstract List getRetainedDetachedExpressions(); + /** * Creates a builder for {@link ExtendedExpression}. * diff --git a/core/src/main/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverter.java b/core/src/main/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverter.java index 7f88bfce6..5a9020e1a 100644 --- a/core/src/main/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverter.java +++ b/core/src/main/java/io/substrait/extendedexpression/ExtendedExpressionProtoConverter.java @@ -8,7 +8,6 @@ import io.substrait.relation.AggregateFunctionProtoConverter; import io.substrait.type.proto.TypeProtoConverter; import io.substrait.util.EmptyVisitationContext; -import java.util.Objects; /** * Converts from {@link io.substrait.extendedexpression.ExtendedExpression} to {@link @@ -29,10 +28,14 @@ public ExtendedExpressionProtoConverter() { /** * Creates a converter that bounds the nesting depth of its output with the given detacher. * - * @param nestingDetacher bounds the nesting depth of each {@link ExtendedExpression} + * @param nestingDetacher bounds the nesting depth of each {@link ExtendedExpression}, must not be + * null */ public ExtendedExpressionProtoConverter(NestingDetacher nestingDetacher) { - this.nestingDetacher = Objects.requireNonNull(nestingDetacher, "nestingDetacher"); + if (nestingDetacher == null) { + throw new IllegalArgumentException("NestingDetacher is required"); + } + this.nestingDetacher = nestingDetacher; } /** @@ -92,6 +95,8 @@ public ExtendedExpression toProto( if (extendedExpression.getAdvancedExtension().isPresent()) { builder.setAdvancedExtensions(extendedExpression.getAdvancedExtension().get()); } + // Ahead of any newly detached entry, so the advanced extension's ordinals keep their targets. + builder.addAllDetachedExpressions(extendedExpression.getRetainedDetachedExpressions()); return nestingDetacher.detach(builder.build()); } } diff --git a/core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java b/core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java index d37cf8006..d98308208 100644 --- a/core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java +++ b/core/src/main/java/io/substrait/extendedexpression/ProtoExtendedExpressionConverter.java @@ -60,13 +60,10 @@ public ExtendedExpression from(io.substrait.proto.ExtendedExpression extendedExp io.substrait.type.NamedStruct namedStruct = io.substrait.type.NamedStruct.fromProto(baseSchemaProto, protoTypeConverter); + DetachedSubtrees detachedSubtrees = DetachedSubtrees.of(extendedExpression); ProtoExpressionConverter protoExpressionConverter = new ProtoExpressionConverter( - functionLookup, - this.extensionCollection, - namedStruct.struct(), - null, - DetachedSubtrees.of(extendedExpression)); + functionLookup, this.extensionCollection, namedStruct.struct(), null, detachedSubtrees); List expressionReferences = new ArrayList<>(); @@ -116,6 +113,10 @@ public ExtendedExpression from(io.substrait.proto.ExtendedExpression extendedExp ? extendedExpression.getAdvancedExtensions() : null)) .baseSchema(namedStruct); + if (extendedExpression.hasAdvancedExtensions()) { + // The advanced extension stays raw, so the entries its ordinals may name keep their ordinals. + builder.retainedDetachedExpressions(detachedSubtrees.unresolvedPrefix()); + } return builder.build(); } diff --git a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java index 5a1df2cc7..b94e8eeac 100644 --- a/core/src/main/java/io/substrait/plan/PlanProtoConverter.java +++ b/core/src/main/java/io/substrait/plan/PlanProtoConverter.java @@ -24,6 +24,9 @@ public class PlanProtoConverter { @NonNull private final NestingDetacher nestingDetacher; + /** The spec release that introduced detached expressions, as major, minor and patch. */ + private static final int[] DETACHED_EXPRESSIONS_SINCE = {0, 105, 0}; + /** Default constructor. */ public PlanProtoConverter() { this(DefaultExtensionCatalog.DEFAULT_COLLECTION); @@ -97,19 +100,22 @@ public PlanProtoConverter( */ public Plan toProto(final io.substrait.plan.Plan plan) { final List planRels = new ArrayList<>(); + boolean detached = false; final ExtensionCollector functionCollector = new ExtensionCollector(extensionCollection); for (final io.substrait.plan.Plan.Root root : plan.getRoots()) { final Rel input = new RelProtoConverter(functionCollector, extensionProtoConverter) .toProto(root.getInput()); - planRels.add( - nestingDetacher.detach( - PlanRel.newBuilder() - .setRoot( - io.substrait.proto.RelRoot.newBuilder() - .setInput(input) - .addAllNames(root.getNames())) - .build())); + final PlanRel planRel = + PlanRel.newBuilder() + .setRoot( + io.substrait.proto.RelRoot.newBuilder() + .setInput(input) + .addAllNames(root.getNames())) + .build(); + final PlanRel bounded = nestingDetacher.detach(planRel); + detached |= bounded != planRel; + planRels.add(bounded); } final Plan.Builder builder = Plan.newBuilder() @@ -129,6 +135,14 @@ public Plan toProto(final io.substrait.plan.Plan plan) { plan.getVersion().getGitHash().ifPresent(gh -> versionBuilder.setGitHash(gh)); plan.getVersion().getProducer().ifPresent(p -> versionBuilder.setProducer(p)); + if (detached && precedesDetachedExpressions(versionBuilder)) { + // The git hash identifies the earlier release, so it no longer describes the plan. + versionBuilder + .setMajorNumber(DETACHED_EXPRESSIONS_SINCE[0]) + .setMinorNumber(DETACHED_EXPRESSIONS_SINCE[1]) + .setPatchNumber(DETACHED_EXPRESSIONS_SINCE[2]) + .clearGitHash(); + } builder.setVersion(versionBuilder); @@ -138,6 +152,22 @@ public Plan toProto(final io.substrait.plan.Plan plan) { return builder.build(); } + /** + * Whether a plan declaring {@code version} predates detached expressions, which a {@link PlanRel} + * rewritten by the {@link NestingDetacher} uses, so the declared version has to be raised. + */ + private static boolean precedesDetachedExpressions(final Version.Builder version) { + final int[] declared = { + version.getMajorNumber(), version.getMinorNumber(), version.getPatchNumber() + }; + for (int i = 0; i < declared.length; i++) { + if (declared[i] != DETACHED_EXPRESSIONS_SINCE[i]) { + return declared[i] < DETACHED_EXPRESSIONS_SINCE[i]; + } + } + return false; + } + /** * Converts an {@link io.substrait.plan.Plan.ExecutionBehavior} to its protobuf representation. * diff --git a/core/src/main/java/io/substrait/relation/ProtoRelConverter.java b/core/src/main/java/io/substrait/relation/ProtoRelConverter.java index a2fee09a0..04a454919 100644 --- a/core/src/main/java/io/substrait/relation/ProtoRelConverter.java +++ b/core/src/main/java/io/substrait/relation/ProtoRelConverter.java @@ -205,8 +205,7 @@ public Plan.Root from(io.substrait.proto.PlanRel planRel) { throw new IllegalArgumentException( "Only PlanRel roots are supported, got " + planRel.getRelTypeCase()); } - DetachedSubtrees enclosing = this.detachedSubtrees; - this.detachedSubtrees = DetachedSubtrees.of(planRel); + DetachedSubtrees enclosing = enter(planRel); try { return from(planRel.getRoot()); } finally { @@ -214,11 +213,45 @@ public Plan.Root from(io.substrait.proto.PlanRel planRel) { } } + /** + * Converts the bare relation of a {@code PlanRel}, resolving the detached ordinals in it against + * that {@code PlanRel}'s detached subtrees. + * + * @param planRel the protobuf {@code PlanRel} to convert, which must hold a bare relation + * @return the converted relation + * @throws IllegalArgumentException if {@code planRel} holds a root rather than a bare relation + */ + public Rel fromRel(io.substrait.proto.PlanRel planRel) { + if (!planRel.hasRel()) { + throw new IllegalArgumentException( + "Expected a PlanRel holding a bare relation, got " + planRel.getRelTypeCase()); + } + DetachedSubtrees enclosing = enter(planRel); + try { + return from(planRel.getRel()); + } finally { + this.detachedSubtrees = enclosing; + } + } + + /** + * Resolves ordinals against {@code planRel} from now on, returning what they resolved against. + */ + private DetachedSubtrees enter(io.substrait.proto.PlanRel planRel) { + DetachedSubtrees enclosing = this.detachedSubtrees; + this.detachedSubtrees = DetachedSubtrees.of(planRel); + return enclosing; + } + /** * Returns the detached subtrees that expressions converted for the current relation resolve their - * ordinals against: those of the {@code PlanRel} passed to {@link - * #from(io.substrait.proto.PlanRel)}, or a resolver that rejects every ordinal outside such a - * conversion. + * ordinals against: those of the {@code PlanRel} being converted by {@link + * #from(io.substrait.proto.PlanRel)} or {@link #fromRel(io.substrait.proto.PlanRel)}, or a + * resolver that rejects every ordinal outside such a conversion. + * + *

The resolver records each entry it resolves so that it can reject a second reference to it, + * so it is meant for the expression converters of the current conversion only: resolving an + * ordinal through it from elsewhere makes the conversion's own reference to that entry fail. * * @return the current detached subtrees */ diff --git a/core/src/test/java/io/substrait/type/proto/DetachedExpressionRoundtripTest.java b/core/src/test/java/io/substrait/type/proto/DetachedExpressionRoundtripTest.java index 7d6b26f62..b05059d41 100644 --- a/core/src/test/java/io/substrait/type/proto/DetachedExpressionRoundtripTest.java +++ b/core/src/test/java/io/substrait/type/proto/DetachedExpressionRoundtripTest.java @@ -14,6 +14,7 @@ import io.substrait.expression.FieldReference; import io.substrait.expression.LambdaBuilder; import io.substrait.expression.proto.NestingDetacher; +import io.substrait.expression.proto.ProtoExpressionConverter; import io.substrait.extendedexpression.ExtendedExpression; import io.substrait.extendedexpression.ExtendedExpressionProtoConverter; import io.substrait.extendedexpression.ImmutableExpressionReference; @@ -24,8 +25,11 @@ import io.substrait.plan.Plan; import io.substrait.plan.PlanProtoConverter; import io.substrait.plan.ProtoPlanConverter; +import io.substrait.proto.FilterRel; import io.substrait.proto.PlanRel; +import io.substrait.relation.Filter; import io.substrait.relation.NamedScan; +import io.substrait.relation.ProtoRelConverter; import io.substrait.relation.Rel; import io.substrait.relation.Rel.Remap; import io.substrait.type.NamedStruct; @@ -34,6 +38,7 @@ import io.substrait.utils.StringHolderHandlingRelProtoConverter; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; @@ -49,8 +54,7 @@ @TestInstance(TestInstance.Lifecycle.PER_CLASS) class DetachedExpressionRoundtripTest extends StringHolderRoundtripTestBase { - /** A detacher with no depth to spare, so it detaches every expression it can. */ - static final NestingDetacher DETACH_ALL = NestingDetacher.withMaxDepth(0); + static final NestingDetacher DETACH_ALL = NestingDetacher.everyExpression(); /** Deep enough that the nested additions exceed protobuf-java's parse recursion limit. */ static final int DEEP = 200; @@ -199,6 +203,132 @@ void anOrdinalOutsideAContainerIsRejected() { assertTrue(error.getMessage().contains("outside a PlanRel"), error.getMessage()); } + @Test + void aShallowSubqueryOverALongChainIsDetachedByTheFallback() + throws InvalidProtocolBufferException { + Rel subquery = filters(scan, 28); + Rel project = + sb.project( + input -> List.of(sb.scalarSubquery(subquery, R.I32)), Remap.of(List.of(1)), scan); + Plan plan = sb.plan(sb.root(filters(project, 18))); + + io.substrait.proto.Plan undetached = planConverter(NestingDetacher.disabled()).toProto(plan); + assertTrue(depth(undetached) > NestingDetacher.DEFAULT_MAX_DEPTH); + io.substrait.proto.Plan proto = + io.substrait.proto.Plan.parseFrom(new PlanProtoConverter().toProto(plan).toByteArray()); + assertTrue(depth(proto) <= NestingDetacher.DEFAULT_MAX_DEPTH, () -> "depth " + depth(proto)); + assertEquals(plan, new ProtoPlanConverter().from(proto)); + } + + @Test + void aContainerThatCannotBeMadeToFitIsReturnedAsIs() { + PlanRel planRel = planRel(filters(scan, 60)); + assertTrue(depth(planRel) > NestingDetacher.DEFAULT_MAX_DEPTH); + assertSame(planRel, NestingDetacher.defaultDetacher().detach(planRel)); + } + + @Test + void detachingRaisesAnEarlierDeclaredVersion() { + Plan.Version earlier = + Plan.Version.builder() + .major(0) + .minor(90) + .patch(0) + .gitHash(Optional.of("0123abc")) + .producer(Optional.of("a producer")) + .build(); + Plan deep = + Plan.builder() + .from(sb.plan(sb.root(sb.project(in -> List.of(nestedAdditions(in, DEEP)), scan)))) + .version(earlier) + .build(); + io.substrait.proto.Version written = new PlanProtoConverter().toProto(deep).getVersion(); + assertEquals( + List.of(0, 105, 0), + List.of(written.getMajorNumber(), written.getMinorNumber(), written.getPatchNumber())); + assertEquals("", written.getGitHash()); + assertEquals("a producer", written.getProducer()); + + Plan shallow = + Plan.builder() + .from(sb.plan(sb.root(sb.project(in -> List.of(nestedAdditions(in, 3)), scan)))) + .version(earlier) + .build(); + io.substrait.proto.Version unchanged = new PlanProtoConverter().toProto(shallow).getVersion(); + assertEquals(90, unchanged.getMinorNumber()); + assertEquals("0123abc", unchanged.getGitHash()); + } + + @Test + void anOpaqueAdvancedExtensionKeepsItsDetachedTargets() { + io.substrait.proto.ExtendedExpression shallow = + new ExtendedExpressionProtoConverter() + .toProto( + ImmutableExtendedExpression.builder() + .baseSchema(NamedStruct.of(List.of("a"), R.struct(R.I32))) + .addReferredExpressions( + ImmutableExpressionReference.builder() + .expression(sb.equal(sb.fieldReference(scan, 0), sb.i32(1))) + .addOutputNames("eq") + .build()) + .build()); + io.substrait.proto.Expression payloadTarget = literal(); + // The payload names entry 0; the referred expression names entry 1, which is resolved. + io.substrait.proto.ExtendedExpression withPayload = + shallow.toBuilder() + .setReferredExpr(0, shallow.getReferredExpr(0).toBuilder().setExpression(ordinal(1))) + .addDetachedExpressions(payloadTarget) + .addDetachedExpressions(shallow.getReferredExpr(0).getExpression()) + .setAdvancedExtensions( + io.substrait.proto.AdvancedExtension.newBuilder() + .setEnhancement(com.google.protobuf.Any.pack(ordinal(0)))) + .build(); + + ExtendedExpression read = new ProtoExtendedExpressionConverter().from(withPayload); + assertEquals(List.of(payloadTarget), read.getRetainedDetachedExpressions()); + + io.substrait.proto.ExtendedExpression written = + new ExtendedExpressionProtoConverter().toProto(read); + assertEquals(List.of(payloadTarget), written.getDetachedExpressionsList()); + assertEquals(read, new ProtoExtendedExpressionConverter().from(written)); + + io.substrait.proto.ExtendedExpression allDetached = + new ExtendedExpressionProtoConverter(DETACH_ALL).toProto(read); + assertEquals(payloadTarget, allDetached.getDetachedExpressions(0)); + assertEquals(read, new ProtoExtendedExpressionConverter().from(allDetached)); + } + + @Test + void aBarePlanRelIsReadWithItsDetachedSubtrees() { + Rel rel = filterOn(sb.equal(sb.fieldReference(scan, 0), sb.i32(1))); + PlanRel detached = + DETACH_ALL.detach(PlanRel.newBuilder().setRel(relProtoConverter.toProto(rel)).build()); + assertTrue(detached.getDetachedExpressionsCount() > 0); + assertEquals(rel, protoRelConverter.fromRel(detached)); + assertThrows(IllegalArgumentException.class, () -> protoRelConverter.from(detached)); + } + + @Test + void anExpressionConverterBuiltBeforeTheConversionResolvesAgainstIt() { + ProtoRelConverter reader = + new ProtoRelConverter(functionCollector, extensions) { + // Built before any PlanRel is converted, as a subclass field would be. + final ProtoExpressionConverter early = + new ProtoExpressionConverter( + functionCollector, extensions, scan.getRecordType(), this); + + @Override + protected Filter newFilter(FilterRel rel) { + return Filter.builder() + .input(from(rel.getInput())) + .condition(early.from(rel.getCondition())) + .build(); + } + }; + Rel rel = filterOn(sb.equal(sb.fieldReference(scan, 0), sb.i32(1))); + assertEquals(rel, reader.from(DETACH_ALL.detach(planRel(rel))).getInput()); + } + @Test void aPlanRelWithoutARootIsRejected() { IllegalArgumentException error = @@ -337,6 +467,16 @@ private void assertRejected( assertTrue(error.getMessage().contains(message), error.getMessage()); } + /** {@code count} filters stacked on {@code input}. */ + private Rel filters(Rel input, int count) { + Rel rel = input; + for (int i = 0; i < count; i++) { + int value = i; + rel = sb.filter(in -> sb.equal(sb.fieldReference(in, 0), sb.i32(value)), rel); + } + return rel; + } + private Rel filterOn(Expression condition) { return sb.filter(input -> condition, scan); } diff --git a/spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala b/spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala index dc0d09c4f..d50f1ca72 100644 --- a/spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala +++ b/spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala @@ -45,6 +45,7 @@ import io.substrait.{proto, relation} import io.substrait.debug.TreePrinter import io.substrait.expression.{Expression => SExpression, ExpressionCreator} import io.substrait.expression.Expression.StructLiteral +import io.substrait.expression.proto.NestingDetacher import io.substrait.extension.ExtensionCollector import io.substrait.hint.Hint import io.substrait.plan.{ImmutableExecutionBehavior, Plan} @@ -899,14 +900,14 @@ class ToSubstraitRel extends AbstractLogicalPlanVisitor with Logging with Predic val extensionCollector = new ExtensionCollector val relProtoConverter = new RelProtoConverter(extensionCollector) + val planRel = proto.PlanRel + .newBuilder() + .setRel(substraitRel + .accept(relProtoConverter, EmptyVisitationContext.INSTANCE)) + .build() val builder = proto.Plan .newBuilder() - .addRelations( - proto.PlanRel - .newBuilder() - .setRel(substraitRel - .accept(relProtoConverter, EmptyVisitationContext.INSTANCE)) - ) + .addRelations(NestingDetacher.defaultDetacher().detach(planRel)) extensionCollector.addExtensionsToPlan(builder) builder.build().toByteArray }