Repository navigation
feat(core)!: bump substrait to v0.105.0 - #1415
nielspardon wants to merge 2 commits into
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
|
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.
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)does the same for a singlePlanRel(and refuses a bare-relationPlanRelwith an accurate message). An ordinal converted outside a container is rejected.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. Ordinals are assigned in pre-order. The detacher is on by default; newPlanProtoConverter/ExtendedExpressionProtoConverterconstructors takeNestingDetacher.withMaxDepth(n)ordisabled(). 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. The read side inlines resolved entries and keeps no table. An ordinal inside a
google.protobuf.Anypayload that a custom converter keeps without interpreting would lose its target. The default converters keep no such payloads.The design was aligned with the substrait-python implementation (substrait-io/substrait-python#317): same depth counting, cut rule and ordinal order. 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.