Repository navigation
feat: read and write detached expression subtrees - #317
Conversation
Bumps substrait-protobuf, substrait-extensions, and substrait-antlr to spec v0.105.0 and carries the new file-level extension `description` through build_simple_extensions. BREAKING CHANGE: `avg` over a decimal now derives a nullable `DECIMAL?<38,S>` result, following spec v0.105.0. Plans built with this release declare the aggregate's output type nullable.
Spec v0.105.0 lets a producer move expression subtrees into `detached_expressions` on the containing `PlanRel` or `ExtendedExpression`, so plans with deep expressions stay within protobuf's parse recursion limit (100 levels in upb, Java and C++). `detach_expressions` cuts expressions that would exceed a nesting budget, and `resolve_detached_expressions` inlines the ordinals, enforcing the spec's validation rules. Schema inference, the plan printer and the builders resolve detached expressions on their own.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Summary
Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 29.51% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 61 functions across 6 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Title check The title clearly and concisely summarizes the main change: support for reading and writing detached expression subtrees. Description check The description provides the rationale, implementation details, behavior, limitations, error cases, and compatibility context for the detached expression subtree support.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 29.51% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 61 functions across 6 files. (1 skipped: 1 unsupported.)
- 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.
|
Stacked on #316 (the v0.105.0 bump, which brings in the new proto fields). Until that merges, this diff also contains its commit; review only the last commit. |
Rename the API to detach / resolve_detached in substrait.utils.detached and drive it from a per-table description, so detached relations (substrait#1236) become one more table. Resolve inner plans in the subquery builders, screen for ordinals on the wire bytes before walking, keep only entries an opaque payload may still reference, raise a clear error on a bare ordinal, and let the plan printer show malformed plans.
|
@coderabbitai resume |
|
andrew-coleman
left a comment
andrew-coleman
left a comment
There was a problem hiding this comment.
The encoder looks solid: a randomized check of detach over 300 trees found no bad round trips. The inline comments are about how the builders use detached input, plus a speed cost on every build.
Breaking change. This PR includes #316's chore(deps)! commit, but the squash commit takes its message from this PR's title and description, so the decimal avg change would ship with no ! and no release note. If #316 is closed in favour of this PR, the title needs feat!: and the description a footer as its last paragraph, unwrapped. For example:
BREAKING CHANGE: `avg` over a decimal now derives a nullable `DECIMAL?<38,S>` result, following spec v0.105.0, so plans built with this release declare the aggregate's output type nullable. The pinned substrait-extensions 0.105.0 also makes `description` the second field of `SimpleExtensions`, ahead of `dependencies`, so code that builds one positionally must pass `dependencies` by keyword.
PR description. It ends with 🤖 Generated with AI. The description becomes the squash-merge commit body, and CONTRIBUTING.md:114 asks to keep tool-attribution lines out of it. Once there's a footer, anything after it is also published as part of the breaking-change note.
Printer. Ordinals used as function arguments print as <unknown_function_argument_value>, because _get_function_argument_string (display.py:694) and _stream_function_argument (display.py:745) check the argument's type themselves. Both need the same $detached[n] branch as _stream_expression. The detached table is never printed, and the except ValueError at display.py:138 covers the whole plan, so one malformed PlanRel leaves the other valid ones unresolved too.
Smaller
detached.py:102:field.message_type.GetOptions().map_entryruns for every repeated field in every walk.hasattr(value, "values"), asutils/__init__.py:332does, avoids the descriptor lookup.detached.py:297:detachwalks the tree twice, first in_exceedsand then when_detach_in_placeindexes it, which already computesheight.detached.py:305-306:_resolve_in_placeloops over_TABLES, but_detach_in_placehard-codes_EXPRESSIONS, so adding a second table would still mean rewriting the writer.tests/test_detached.py:106-111:_submessagesyields a map's keys instead of its values, and counts a map value as one level instead of two. No message-valued map is reachable fromPlanorExtendedExpressiontoday, so this is latent.
| `detach` returns a plan already within the limit as is. `infer_plan_schema`, | ||
| `infer_extended_expression_schema`, the plan printer, and the builders resolve detached expressions | ||
| on their own. Functions that take a bare `Rel` or `Expression` cannot, since the table lives on the | ||
| containing message, so resolve the `Plan` or `ExtendedExpression` first. |
There was a problem hiding this comment.
The builders can't use the plans that most need detaching. Inlining rebuilds the deep tree, and the builders pass it to upb message constructors. Those copy through the parser and hit the same 100-level limit, as _deep_plan's docstring notes. I built a 60-level scalar-function chain in place, then detached, serialized and parsed it. It resolves, and infer_plan_schema works on it, but select(parsed, [column("a")]) and fetch(parsed, None, None) raise DecodeError: Error parsing message with type 'substrait.Rel'. The full fix is to carry the table through the builders and renumber its ordinals, the way _merge_input_subtrees and rebase_reference_ordinals already handle ReferenceRel ordinals. Until then, this paragraph should say the builders only accept detached plans that still fit the limit once inlined.
There was a problem hiding this comment.
Narrowed the README: builders only accept detached plans that still fit the limit once inlined, and the finished plan should be detached just before serializing. Carrying the tables through the builders and renumbering them like ReferenceRel subtrees is now #319, which also covers builder chains failing past 47 steps on main.
| def _holds_ordinals(container) -> bool: | ||
| """Whether ``container``'s relation or referred expressions hold an ordinal. | ||
|
|
||
| Screened on the wire bytes first: serialization is canonical, so if no | ||
| ordinal field's tag occurs in them there is no ordinal, and that is decided | ||
| in native code. Only when a tag's bytes do occur -- an ordinal, or the same | ||
| bytes inside some other value -- does the descriptor walk settle it. | ||
| """ | ||
| data = container.SerializeToString() |
There was a problem hiding this comment.
The byte check often fires falsely. Its tag bytes 0x90 0x01 are also the varint 144, for example the length of any 144-byte submessage. 127 of 199 builder-made select plans (1–199 columns) trip it and fall back to the walk in Python. infer_plan_schema runs this on every builder step. On a 199-column plan with no detached expressions, resolve_detached takes 0.83 ms of infer_plan_schema's 3.5 ms. Nothing this library builds has a table, so skipping the check when a container has none brings it down to about 1 µs:
| def _holds_ordinals(container) -> bool: | |
| """Whether ``container``'s relation or referred expressions hold an ordinal. | |
| Screened on the wire bytes first: serialization is canonical, so if no | |
| ordinal field's tag occurs in them there is no ordinal, and that is decided | |
| in native code. Only when a tag's bytes do occur -- an ordinal, or the same | |
| bytes inside some other value -- does the descriptor walk settle it. | |
| """ | |
| data = container.SerializeToString() | |
| def _holds_ordinals(container) -> bool: | |
| """Whether ``container``'s relation or referred expressions hold an ordinal. | |
| A container with no detached entries is taken to hold none: nothing this | |
| library builds has any, so this is the hot path, and an ordinal with no | |
| entry to index is invalid anyway (type inference rejects it). Otherwise the | |
| wire bytes are screened first, in native code, and only when an ordinal | |
| field's tag occurs in them does the descriptor walk settle it. | |
| """ | |
| if not any(getattr(container, table.field) for table in _TABLES): | |
| return False | |
| data = container.SerializeToString() |
The trade-off is that resolve_detached would no longer reject an ordinal in a container with no table, so the out of range case of test_resolve_rejects_invalid_references would need changing. Resolving once where plans enter the builders, instead of again in every infer_* call, would keep that check.
There was a problem hiding this comment.
Took the no-table fast path for the builders and infer_* through a separate helper (resolve_present_tables). The public resolve_detached stays strict, so it still rejects an ordinal with no table, and on the hot path schema inference rejects one with a ValueError that points at resolve_detached.
…ssions # Conflicts: # pixi.lock # pyproject.toml # uv.lock
Resolve plans and expressions that builders receive from callables, refuse to build on entries an opaque payload may still reference, blank resolved entries kept in place, and skip the hot-path check when a message has no detached entries. Walk fields in declaration order and existing entries first, as substrait-java does, make the writer generic over tables, and let the printer show detached function arguments and each malformed PlanRel on its own.
2 participants
Implements bounded expression nesting from spec v0.105.0 (substrait-io/substrait#1218). Protobuf parsers reject messages nested more than 100 levels below the root, in upb (the Python runtime) as well as Java and C++. Each expression level costs several message levels, so a nested-struct expression only about 33 levels deep already fails to parse in Python. Moving subtrees into
detached_expressionskeeps the encoding shallow.New module
substrait.utils.detached:resolve_detached(Plan | PlanRel | ExtendedExpression)inlines every ordinal eagerly and raisesValueErroron a spec violation in the reachable portion: an ordinal out of range or pointing at another container, an entry referenced twice, a cycle, or an entry that is itself an ordinal. Unreachable entries are not validated.infer_plan_schema,infer_extended_expression_schema, the plan printer, and the builders (subquery builders included) resolve on their own, so a detached plan can be used like any other as long as it still fits the parse limit once inlined. Builders refuse a plan whose leftover entries an extension payload may still reference, since the table does not travel into the plan they build. Functions that take a bareRelorExpressionhave no table to resolve against and raise aValueErrorpointing atresolve_detached. A message without ordinals is returned as is.detach(..., max_depth=100)is opt-in, since builders hand back protos and callers serialize them. Depth is counted in message levels below the root, the way the parsers count it, and a message within budget is returned as is. Otherwise the walk is top-down, through existing entries first, then the relation or referred expressions, in field declaration order: an expression is detached when it sits deeper thanmax_depth // 2(and below the entry roots) and its subtree would exceed the budget. Entries are numbered in pre-order, and entries already in the table keep their positions. The result is best effort: nesting in relation trees, literals, types, andAnypayloads is left as is.max_depth=0detaches everything it can, which is useful for testing consumers.The cut rule, depth counting, and pre-order numbering match the design being implemented in substrait-java, so both bindings produce the same encoding. One difference: when a container carries a
google.protobuf.Anypayload, resolution keeps the entries it did not inline, at their ordinals; it drops the trailing run it did inline and replaces the rest of those with a null literal. An opaque extension payload may reference unreachable entries, and the spec forbids removing or renumbering those; it cannot validly reference an inlined one, since each entry is referenced at most once. Without such a payload the table is cleared, as in Java.The names are table-neutral, and the walker is driven by a per-table description, so detached relation subtrees (substrait-io/substrait#1236) can be added as a second table.