Repository navigation
feat(core)!: bump substrait to v0.105.0 - #1415
Conversation
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 substrait-io#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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 110 functions across 13 files. (1 skipped… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check The PR satisfies the coding requirements in directly linked issue #1352.gradle/libs.versions.tomlupdatessubstrait-packagingto0.105.0.DetachedSubtreesandNestingDetacherimplement deta…Out of Scope Changes check The changed files remain within issue #1352. Converter and detacher changes implement detached expression subtree support. Tests cover conversion, validation, round trips, depth handling, and version …Title check The title is concise, specific, and accurately summarizes the v0.105.0 upgrade and related breaking changes. It follows the required Conventional Commit format. Description check The description provides a detailed rationale, implementation summary, compatibility impact, linked issue context, and a correctly labeled BREAKING CHANGE footer. It satisfies the required template. Full details: Linked Issues check
-
Explanation
The PR satisfies the coding requirements in directly linked issue
#1352.gradle/libs.versions.tomlupdatessubstrait-packagingto0.105.0.DetachedSubtreesandNestingDetacherimplement detached-expression reading, validation, writing, and configurable depth handling. The changes preserve required detached entries for advanced extensions and update the declared version when detachment occurs. The extension parser already ignores unknown file-leveldescriptionkeys. The decimalavgbehavior is represented by the v0.105.0 artifact update and the updated nullable test documentation.
Full details: Out of Scope Changes check
-
Explanation
The changed files remain within issue
#1352. Converter and detacher changes implement detached expression subtree support. Tests cover conversion, validation, round trips, depth handling, and version behavior. The Spark integration, artifact version update, decimalavgdocumentation, and retained-entry handling support the release requirements. No unrelated change is demonstrated.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 110 functions across 13 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
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.
andrew-coleman
left a comment
andrew-coleman
left a comment
There was a problem hiding this comment.
The read side looks right to me: resolving entries in place, and validating only the reachable part of the table, both match the spec. The depth boundary also checks out: 30 nested adds give a plan 98 levels deep, left unchanged and parseable, and 31 give 101, detached down to 54 and parseable. I'd like the first three inline comments addressed before merge (the ExtendedExpression payload, the cut rule, and run() not checking its result). The rest are smaller.
One thing that isn't on a line in this diff:
Spark producer path. ToSubstraitRel.toProtoSubstrait (spark/src/main/scala/io/substrait/spark/logical/ToSubstraitRel.scala:897) builds proto.Plan/PlanRel by hand with RelProtoConverter, so it never goes through NestingDetacher. A Spark plan with about 30 or more levels of nested arithmetic still serializes to bytes that Plan.parseFrom rejects, so the description's claim doesn't cover that path. It also emits a bare-rel PlanRel, which ProtoPlanConverter can't read. new PlanProtoConverter().toProto(convert(p)).toByteArray would fix both, at the cost of emitting root instead of rel. A follow-up is fine if you'd rather keep this PR to core.
| this.extensionCollection, | ||
| namedStruct.struct(), | ||
| null, | ||
| DetachedSubtrees.of(extendedExpression)); |
There was a problem hiding this comment.
This inlines and drops detached_expressions, but advanced_extensions is kept as the raw proto (ExtendedExpression.getAdvancedExtension() is an io.substrait.proto.AdvancedExtension). ExtendedExpressionProtoConverter (lines 92-95) then writes it back next to a table the detacher builds from scratch. An ordinal inside that payload either dangles (shallow expressions, so no table) or names whatever subtree ended up at that index in the new pre-order.
The spec rules this out: "When retaining opaque extension payloads, consumers must not remove or renumber detached entries unless they can ensure that references inside those payloads continue to identify the same expression subtrees." It also contradicts the DetachedSubtrees Javadoc and the description's "The default converters keep no such payloads."
Pass already keeps existing entries at their indexes, so one fix: when advanced_extensions is present, carry the original detached_expressions on the POJO and add them to the builder before detach. The inlined trees no longer reference those entries, which is allowed, and the payload's ordinals keep their targets.
There was a problem hiding this comment.
Fixed: the POJO keeps every entry up to the last one that wasn't resolved (getRetainedDetachedExpressions), and the writer adds them back before detaching, so the payload's ordinals keep their targets. Dropping only the trailing run of resolved entries keeps the table from growing on repeated round trips.
| return message; | ||
| } | ||
| if (message instanceof io.substrait.proto.Expression | ||
| && depth > Math.max(maxDepth / 2, entryDepth()) |
There was a problem hiding this comment.
With depth > Math.max(maxDepth / 2, entryDepth()), an expression at depth 50 or less is never detached, even when detaching it is the only thing that would make the container fit.
Example: 18 filters over a project whose expression is a scalar subquery over 28 more filters. The subquery expression is at depth 41 and the plan is 107 deep. The default detacher moves 2 filter conditions, the result is still 107 deep, and Plan.parseFrom throws. Detaching the subquery expression itself would put it at depth 2 and bring the maximum to about 68.
The class Javadoc says chains of relations aren't shortened, but this one is fixable with an expression cut. Could the pass fall back to a lower threshold (down to entryDepth()) when the first attempt doesn't fit? Since the rule was aligned with substrait-python, it may be worth raising there too.
There was a problem hiding this comment.
Fixed: if the first pass doesn't fit, it retries with any expression below the container eligible. Your 18 + 28 filter example is now a test, and I've told the substrait-python side.
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| <M extends Message> M run(M container) { |
There was a problem hiding this comment.
run() returns the rewritten container without checking that it now fits. Each expression is judged on its own, so in three sizes of the example on line 160, 2 to 6 entries were detached while the depth stayed at 107, 111 and 115. The output still fails default parsers, and it now needs a v0.105.0 consumer too, which is worse for a consumer that had raised its limit. Could run() check the result with within(...) and return container unchanged when it still doesn't fit, after trying the fallback from the line-160 comment?
There was a problem hiding this comment.
Fixed: a container that still doesn't fit after both attempts is returned unchanged.
| extensions, | ||
| rootType, | ||
| relConverter, | ||
| relConverter == null ? DetachedSubtrees.none() : relConverter.detachedSubtrees()); |
There was a problem hiding this comment.
This takes relConverter.detachedSubtrees() once, at construction. That's documented, but it's easy to get wrong:
- A converter created in a
ProtoRelConvertersubclass's constructor or field (the patternTestBaseuses) holdsnone()and rejects every ordinal, even insidefrom(PlanRel). - A converter created during
from(PlanRel A)and cached resolvesPlanRel B's ordinals against A's table, with no error.
Asking the rel converter when an ordinal is resolved avoids both: keep relConverter and call relConverter.detachedSubtrees() in the DETACHED_EXPRESSION_ORDINAL case when no explicit DetachedSubtrees was passed. A smaller point: detachedSubtrees() is public and returns the stateful resolver, so an outside call to resolveExpression marks an entry as referenced. A later legitimate reference to that entry then fails with "referenced more than once".
There was a problem hiding this comment.
Fixed: the table is now looked up when the ordinal is reached. The accessor's Javadoc now says that resolving through it from outside marks the entry as referenced.
| * @throws IllegalArgumentException if {@code planRel} holds a bare relation rather than a root | ||
| */ | ||
| public Plan.Root from(io.substrait.proto.PlanRel planRel) { | ||
| if (!planRel.hasRoot()) { |
There was a problem hiding this comment.
The spec defines detached_expressions for every PlanRel, a bare rel included. This refuses one, and from(Rel) rejects every ordinal, so there's no way to read a bare-rel PlanRel that uses the table. Plan only models roots, so ProtoPlanConverter couldn't use it anyway, but ProtoRelConverter is public in its own right. Could there be a variant that returns the Rel?
Also, a bare-rel PlanRel passed to ProtoPlanConverter used to fail with UnsupportedOperationException (Unsupported RelTypeCase of REL_NOT_SET) and now fails with IllegalArgumentException. The new message is better, but callers that catch the old type will miss it, so it may be worth a mention since this PR is breaking anyway.
There was a problem hiding this comment.
Added fromRel(PlanRel), and the exception-type change is now in the BREAKING CHANGE footer.
| .setInput(input) | ||
| .addAllNames(root.getNames())) | ||
| .build()); | ||
| nestingDetacher.detach( |
There was a problem hiding this comment.
When the detacher rewrites a PlanRel, the plan version is still copied from the POJO (lines 124-133). Read a deep plan that declares 0.90 (parsed with a raised recursion limit), write it back, and the output says 0.90 while containing detached_expression_ordinals. A consumer that picks its decoder by version will misread it. Could the declared version be raised to at least 0.105.0 when any PlanRel changes? ExtendedExpressionProtoConverter doesn't write a version at all today, so only Plan is affected.
There was a problem hiding this comment.
Fixed: a detached plan now declares at least 0.105.0, with the git hash dropped since it names the earlier release.
| try { | ||
| return within( | ||
| message.getDescriptorForType(), | ||
| CodedInputStream.newInstance(message.toByteArray()), |
There was a problem hiding this comment.
Optional, not blocking: within serializes the whole container on every toProto, shallow plans included, and callers usually serialize it again right after. Your 1.4x measurement may make that acceptable, but plans with a large VirtualTable or many literals pay a second full serialization plus a byte array the size of the PlanRel. Tracking depth in RelProtoConverter/ExpressionProtoConverter while building would make the common case free.
There was a problem hiding this comment.
Leaving this as is. Tracking depth while building would touch every visitor, and custom extension converters can't report their depth. Protobuf also caches the computed size, so the caller's later serialization reuses it.
| * @param nestingDetacher bounds the nesting depth of each {@link ExtendedExpression} | ||
| */ | ||
| public ExtendedExpressionProtoConverter(NestingDetacher nestingDetacher) { | ||
| this.nestingDetacher = Objects.requireNonNull(nestingDetacher, "nestingDetacher"); |
There was a problem hiding this comment.
nit: PlanProtoConverter and ProtoExtendedExpressionConverter throw IllegalArgumentException("... is required") for a null argument, but this throws NullPointerException. Matching them would be nice.
- 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.
|
Spark's |
2 participants
Bumps the
substrait-packagingartifacts to v0.105.0 and implements its main feature, detached expression subtrees (substrait-io/substrait#1218), in both directions. Protobuf parsers reject messages nested more than 100 levels below the root (protobuf-java, C++ and upb alike), and every nested function call costs three levels, so a plan with roughly 30 levels of nested expressions could be serialized by substrait-java but not parsed back, even by substrait-java.ProtoExpressionConverterresolvesdetached_expression_ordinalby converting the entry in place, so the entry binds to the input schema and correlation scope of the referencing site.DetachedSubtreesenforces the spec's rules on the reachable portion of the table (in range, referenced at most once, acyclic, entry not itself an ordinal) and rejects violations withIllegalArgumentException. Unreferenced entries are neither converted nor validated.ProtoPlanConverter/ProtoExtendedExpressionConvertersupply each container's table, and the newProtoRelConverter.from(PlanRel)/fromRel(PlanRel)do the same for a singlePlanRelholding a root or a bare relation. An expression converter asks its rel converter for the current table when it reaches an ordinal. An ordinal converted outside a container is rejected. An extended expression'sadvanced_extensionsis kept as the raw proto, so the entries its ordinals may name are retained on the POJO (getRetainedDetachedExpressions, every entry up to the last unresolved one) and written back at the same ordinals.NestingDetacherruns on eachPlanRelandExtendedExpressionafter POJO → proto. A container that fits within the default limit of 100 is returned unchanged, so only plans that default parsers would reject change. Otherwise an expression that doesn't fit is detached once it lies deeper than half the limit, and its subtree is processed again from its new position directly below the container; if the result is still too deep (for example a shallow subquery over a long relation chain), detachment is retried with every expression below the container eligible, and a container neither attempt makes fit is returned unchanged rather than needing a v0.105.0 consumer for nothing. Ordinals are assigned in pre-order. A plan that gets detached declares at least version 0.105.0. The detacher is on by default; newPlanProtoConverter/ExtendedExpressionProtoConverterconstructors takeNestingDetacher.withMaxDepth(n),disabled(), oreveryExpression()for exercising a consumer. Spark'stoProtoSubstraitgoes through the detacher as well. The fit check scans the serialized bytes rather than walking the tree reflectively; that walk had madetoProtoabout 8x slower on TPC-DS plans, while the scan costs about 1.4x oftoProto+toByteArray.avgover a decimal now returnsDECIMAL?<38,S>(substrait-io/substrait#1253). Isthmus already emitted a nullable average, so only a stale test comment changes.descriptionin extension YAMLs (substrait-io/substrait#1257) is ignored, as the parser already did for unknown top-level keys.The API is named and shaped for both detached tables, so detached relations (substrait-io/substrait#1236) can land without another breaking change. In a plan, the read side inlines resolved entries and keeps no table: the default converters interpret or drop every
Anypayload, and a custom converter must not keep one uninterpreted when it holds ordinals.The design was aligned with the substrait-python implementation (substrait-io/substrait-python#317): same depth counting and ordinal order; the fallback cut and the unchanged-result rule have been shared with it. That one is opt-in, because Python has no POJO → proto step to hook.
Closes #1352
BREAKING CHANGE:
avgover a decimal resolves to a nullableDECIMAL?<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.ProtoPlanConverternow rejects a bare-relationPlanRelwithIllegalArgumentExceptioninstead ofUnsupportedOperationException.