Skip to content

Commit ada41f7

Browse files
committed
feat!: assign extension anchors per plan, not per registry
`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, spelled out in the protos since Substrait v0.83.0), 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 #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 #236
1 parent 532a731 commit ada41f7

18 files changed

Lines changed: 2426 additions & 355 deletions

File tree

CONTRIBUTING.md

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,3 +26,43 @@ Run tests in the project's root dir.
2626
```
2727
uv run pytest
2828
```
29+
30+
## Integration tests
31+
32+
`tests/integration/` holds tests that run against third-party Substrait
33+
implementations, which release on their own schedule: pyarrow as a producer whose
34+
output we consume, and DuckDB and DataFusion as consumers of the plans we build.
35+
36+
The default (`addopts` in `pyproject.toml`) deselects `duckdb` and `datafusion`, so
37+
the command above and CI both skip them: handing a lagging consumer a plan built at a
38+
newer spec version can crash the interpreter natively, which no test run can report,
39+
so a red result there is not even reliably a report. **pyarrow runs by default** --
40+
it produces rather than consumes, so it cannot take the process down, and it is the
41+
only place that would notice pyarrow's output shape drifting away from what the
42+
extension-anchor handling assumes.
43+
44+
Select with `-m`, which replaces the default rather than narrowing it:
45+
46+
```
47+
uv run pytest -m integration # every integration test
48+
uv run pytest -m duckdb # just one integration type
49+
uv run pytest -m "integration and not duckdb" # everything except one
50+
```
51+
52+
Mind that `-m` **replaces** the default expression rather than narrowing it, so a `-m`
53+
you meant as a restriction can widen the selection: `-m "not pyarrow"` re-enables
54+
DuckDB and DataFusion, which is the one thing the default exists to prevent. To drop
55+
pyarrow for a single run, skip `-m` and use `uv run pytest
56+
--ignore=tests/integration/test_pyarrow_producer.py`; to drop it for good, add
57+
`and not pyarrow` to the `addopts`.
58+
59+
Naming a path does not select a deselected marker either -- `uv run pytest
60+
tests/integration/` still reports the engine tests as `deselected` until you pass a
61+
`-m`.
62+
63+
The per-type markers are `pyarrow`, `duckdb`, and `datafusion`; each integration test
64+
carries `integration` plus its own, so any one of them can be switched on or off
65+
independently as those projects catch up. If a pyarrow release starts failing, add
66+
`and not pyarrow` to the `addopts` rather than deleting the tests -- they record what
67+
changed. New tests in `tests/integration/` need both markers, and any new marker has
68+
to be registered in `[tool.pytest.ini_options]`.

pyproject.toml

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,24 @@ dev = ["pytest >= 7.0.0", "substrait-antlr==0.99.0", "pyyaml", "sqloxide", "deep
2525
[tool.pytest.ini_options]
2626
pythonpath = "src"
2727
testpaths = "tests"
28+
markers = [
29+
"integration: exercises a third-party Substrait producer or consumer",
30+
"pyarrow: integration test against pyarrow's Substrait output; runs by default",
31+
"duckdb: integration test against duckdb's Substrait consumer; deselected by default",
32+
"datafusion: integration test against datafusion's Substrait consumer; deselected by default",
33+
]
34+
# The default turns off the two integrations that cannot report their own failure, not
35+
# integration testing as a category. DuckDB and DataFusion consume plans this library
36+
# builds, and handing a lagging consumer a plan built at a newer spec version can crash
37+
# the interpreter natively -- so they must not gate a plain `pytest`, which is what CI
38+
# runs. pyarrow goes the other way (it produces, this library consumes), so it cannot
39+
# take the process down and it currently passes: it runs by default, where it can catch
40+
# a pyarrow release drifting away from the output shape the anchor handling assumes.
41+
# Turn it off by adding `and not pyarrow` here if that day comes.
42+
#
43+
# A `-m` on the command line replaces this one rather than being ANDed with it, so
44+
# `-m integration` runs every integration test and `-m duckdb` runs just that one.
45+
addopts = ["-m", "not duckdb and not datafusion"]
2846

2947
[build-system]
3048
requires = ["setuptools>=61.0.0", "setuptools_scm[toml]>=6.2.0"]

0 commit comments

Comments
 (0)