feat!: assign extension anchors per plan, not per registry - #245
Draft
nielspardon wants to merge 1 commit into
Draft
feat!: assign extension anchors per plan, not per registry#245nielspardon wants to merge 1 commit into
nielspardon wants to merge 1 commit into
Conversation
nielspardon
force-pushed
the
feat/extension-collector
branch
from
August 4, 2026 07:14
b24a5f1 to
7319116
Compare
`ExtensionRegistry` handed out `function_anchor` / `extension_urn_anchor` values at registration time and builders stamped those registry-global numbers into plans. But anchors are plan-local in Substrait, which caused two problems: - Plans were not reproducible. A single-`add` plan emitted `function_anchor: 284` against the default extension set and `4` against a minimal one, because the value encoded how many functions the other YAMLs defined and the order the `functions*.yaml` glob returned them (filesystem order, not sorted). - Extending a plan built elsewhere silently corrupted it. The merge helpers dedupe by identity and document "assumes that there are no collisions", with nothing enforcing it, so a foreign plan already using a given anchor produced two URNs at one anchor and two functions at another -- leaving `function_reference` ambiguous, with no error. Introduce `ExtensionCollector`, which owns those anchors for the duration of one build: function references are allocated on first use from 1, and URN anchors are derived at emit time (nothing outside `SimpleExtensionDeclaration` refers to one). It follows substrait-java's `io.substrait.extension.ExtensionCollector`, including that numbering. The collector reaches builders through a contextvar, as the builders' other per-build state already does (`_rel_anchor_counter`, `outer_schemas`, `anchor_scope`). An incoming materialized plan has its declarations read back to `(urn, name)` identities and its references re-derived rather than trusted, so independently numbered inputs cannot disagree about what a reference means. This is what the SQL translator needs, as it builds a set operation's two sides as separate plans before merging them. Identities come off the declaration rather than a catalog lookup, so a plan naming functions absent from the registry still round-trips. An input declaring two different functions at one anchor is refused rather than silently resolved to one of them. Anchor 0 is re-derived like any other. The spec marks it a valid anchor/reference (substrait-io/substrait#900, carried by the pinned v0.99.0 protos), and pyarrow's `serialize_expressions` numbers from 0, emitting a bare `extension_function { name: "add" }`. Rewriting such a reference needs the remap walk to read reference fields off the descriptor rather than `ListFields()`, which omits default-valued proto3 scalars; the two reference fields that are oneof members are gated on `WhichOneof`, so an absent one is never invented. Emission stays 1-based, as those same protos ask producers to prefer non-zero values. Note this does not extend to `type_variation_anchor`, where 0 remains reserved for the system-preferred variation. Every builder folds its inputs through the collector, `_inner_rel` included: only a bare `Rel` crosses into `Expression.Subquery`, so a pre-built plan's declarations would otherwise stay behind with the discarded plan and leave its references dangling. `aggregate` now refuses a measure that is not an aggregate function, which it previously emitted as a measure that is set but empty -- the one shape where a present message does not imply a real function reference, and so the one shape that reference renumbering could not treat correctly. Because the collector accumulates once per build, the per-level extension merging in the builders is gone rather than optimized: an N-verb chain scanned 230 declarations across 80 merge calls at N=40, and now does none. This is the extension half of substrait-io#207; the schema re-inference half is untouched. `ExtensionRegistry` is now a pure catalog. `lookup_urn` and `FunctionEntry.anchor` are removed (`has_urn` / `urns()` replace the former); the urn->function mapping, signature matching and extension-relation registration are unchanged. The pyarrow tests this adds read real `serialize_expressions` output, so they are coupled to a release this project does not control. Third-party integration tests now live in `tests/integration/` behind per-integration markers (`pyarrow`, `duckdb`, `datafusion`), replacing the undocumented `SUBSTRAIT_ENGINE_TESTS` env var, so any one of them can be switched off on its own as those projects catch up. The default deselects `duckdb` and `datafusion` rather than integration testing as a category: handing a lagging consumer a plan built at a newer spec version can crash the interpreter natively, so a red result there is not reliably a report and must never gate a plain `pytest`. pyarrow only produces, so it cannot take the process down and runs by default, where it can catch pyarrow drifting from the output shape the anchor handling assumes. BREAKING CHANGE: emitted extension anchors are now numbered per plan, so plans compared byte-for-byte against output from an earlier release will differ. Anchors are plan-local by spec, so plan semantics are unaffected. `ExtensionRegistry.lookup_urn` and `FunctionEntry.anchor` are removed; use `has_urn()` / `urns()` for URN membership, and `(entry.urn, str(entry))` as a function's durable identity. `ExtensionCollector.adopt` now raises on an input declaring two different functions at one anchor, and `aggregate` raises on a measure that is not an aggregate function; both previously produced a plan with an ambiguous or dangling function reference. Closes substrait-io#236
nielspardon
force-pushed
the
feat/extension-collector
branch
from
August 4, 2026 08:40
7319116 to
35b0036
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes the extension-merging half of #207 as well (the schema re-inference half is untouched).
Problem
ExtensionRegistryhands outfunction_anchor/extension_urn_anchorvalues at registration time, and builders stamp those registry-global numbers into plans. Anchors are plan-local in Substrait, so this caused two problems.Plans were not reproducible. A single-
addplan emittedfunction_anchor: 284against the default extension set and4against a minimal one — the value encoded how many functions the other default YAMLs defined and the order thefunctions*.yamlglob happened to return them (filesystem order, not sorted). Asubstrait-extensionsbump, or a different machine, shifted every anchor.Extending a plan built elsewhere silently corrupted it.
merge_extension_urns/merge_extension_declarationsdedupe by identity and their docstrings state "Assumes that there are no collisions", with nothing enforcing it. Given a plan already using anchor 10/284 for different entities:function_reference: 284is now ambiguous, and no error is raised.Approach
ExtensionCollectorowns those anchors for the duration of one build. Function references are allocated on first use from 1; URN anchors are derived at emit time, since nothing outsideSimpleExtensionDeclarationrefers to one. This follows substrait-java'sio.substrait.extension.ExtensionCollector, including its first-use numbering and its deferral of URN anchors.The collector reaches builders through a contextvar, as the builders' other per-build state already does (
_rel_anchor_counter,outer_schemas,anchor_scope). Four seams carry the change:build_scopedwrapping each resolver, plus_bind(plans),resolve_expression(expressions) and_inner_rel(subqueries) for adopting inputs._inner_relmatters because only a bareRelcrosses intoExpression.Subquery— a pre-built plan's declarations would otherwise stay behind with the discarded plan and leave its references dangling.An incoming materialized plan has its declarations read back to
(urn, name)identities and its references re-derived rather than trusted, so two independently numbered inputs cannot disagree about what a reference means. This is load-bearing rather than defensive: the SQL translator builds a set operation's two sides as separate plans before merging them, and both number from 1.Identities come off the declaration, not a catalog lookup, so a plan naming functions absent from the registry still round-trips. An input that declares two different functions at one anchor is refused rather than silently resolved to one of them — that ambiguity is the defect this PR exists to remove, so producing it from a merge and accepting it from an input should not have different answers.
Anchor 0
Anchor 0 is re-derived like any other anchor. The spec marks it a valid anchor/reference (substrait#900, carried by the pinned v0.99.0 protos), and pyarrow's
serialize_expressionsnumbers from 0 — a two-expression serialization emits anchors0and1, the first as a bareextension_function { name: "add" }.Rewriting a reference of 0 needs
remap_function_referencesto read reference fields off the descriptor rather thanListFields(), which omits default-valued proto3 scalars and so madefunction_reference: 0invisible. Presence rules make that exact:ScalarFunction/WindowFunction/AggregateFunction/WindowRelFunction.function_referenceSortField.comparison_function_reference(oneofsort_kind)WhichOneof, so aSortFieldusingdirectionnever acquires oneComparisonJoinKey.ComparisonType.custom_function_reference(oneofinner_type)Emission stays 1-based, as those same protos ask producers to prefer non-zero values for ergonomics. This does not extend to
type_variation_anchor, where 0 remains reserved for the system-preferred variation.Treating 0 as ordinary also fixes a case that was silently wrong: two pyarrow expressions folded into one build both emitted at
function_anchor: 0, so each reference resolved ambiguously andmultiplywas readable asadd.Two related corrections
aggregatenow refuses a measure that is not an aggregate function. It previously emitted such a measure as ameasurethat is set but empty — malformed on its own, and the one shape where a present message does not imply a real function reference, so the only shape reference renumbering could not treat correctly.aliasrenames on a copy. It returned the messageresolve_expressionhanded it, which is the caller's own object whenever the remap is empty, so it mutated a caller'sExtendedExpression.ExtensionRegistrybecomes a pure catalog. Its urn→function mapping, signature matching and extension-relation registration are unchanged.Result
Anchors are now dense and plan-local:
Byte-identical across a minimal vs. the full default registry, across repeated builds of the same frame, and under
PYTHONHASHSEED0/1/7/12345.Because the collector accumulates once per build, per-level extension merging is gone rather than optimized. Measured against a
mainworktree over an N-verbprojectchain:main(merge calls / declarations scanned)Wall-clock build time is essentially unchanged (59 → 61 ms at N=40): schema re-inference still dominates, which is #207's other half and out of scope here.
Examples are not covered by CI, so they were diffed against a
mainworktree:pyarrow_exampleandduckdb_exampleprint unchanged output (duckdb executes the re-anchored plan to the same result set), thoughpyarrow_example's plan moves its declaration off anchor 0 andduckdb_example's moves86 → 1.builder_exampleanddataframe_examplediffer only in anchor values (89/284/479 → 1–4); canonicalizing every reference by the(urn, name)it resolves to makes all of their emitted plans identical tomain, with no dangling references.Test layout
The pyarrow tests here read real
serialize_expressionsoutput, so they are coupled to a release this project does not control. Third-party integration tests now live intests/integration/behind per-integration markers (pyarrow,duckdb,datafusion), replacing the undocumentedSUBSTRAIT_ENGINE_TESTSenv var, so any one of them can be switched off on its own as those projects catch up.The default deselects
duckdbanddatafusionrather than integration testing as a category: handing a lagging consumer a plan built at a newer spec version can crash the interpreter natively, so a red result there is not reliably a report and must never gate a plainpytest(which is what CI runs). pyarrow only produces, so it cannot take the process down and runs by default, where it can catch pyarrow drifting from the output shape the anchor handling assumes. Note-mreplaces the default expression rather than narrowing it ---m "not pyarrow"would re-enable the engines -- which CONTRIBUTING.md now spells out along with the selectors.BREAKING CHANGE: emitted extension anchors are numbered per plan, so plans compared byte-for-byte against output from an earlier release will differ. Anchors are plan-local by spec, so plan semantics are unaffected.
ExtensionRegistry.lookup_urnandFunctionEntry.anchorare removed; usehas_urn()/urns()for URN membership, and(entry.urn, str(entry))as a function's durable identity.ExtensionCollector.adoptraises on an input declaring two different functions at one anchor, andaggregateraises on a measure that is not an aggregate function; both previously produced a plan with an ambiguous or dangling function reference.Closes #236
🤖 Generated with AI