timsaucer opened a new pull request, #1741: URL: https://github.com/apache/datafusion-python/pull/1741
# Which issue does this PR close? Closes #1676. Part 4 of 4, stacked on #1740 → #1739 → #1738. Review those first; this PR's diff against #1740 is the fourth commit only. `object_stores`, the one item from #1676 not covered, is tracked separately in #1737 — `datafusion-ffi` has no object store type at all, so there is nothing for a capsule to carry. The reasoning is in https://github.com/apache/datafusion-python/issues/1676#issuecomment-5680959562. # Rationale for this change The last field, and the last of the three getters that take the session. It also removes a small structural wart: `register_catalog_provider` did its capsule import and its insert in one function with no split point, so the bundle path could not reuse it without duplicating the four-way branch that decides what a caller actually handed over. # What changes are included in this PR? **`SessionExtensionComponents.catalog_providers`**, as `(name, provider)` pairs. **`resolve_catalog_provider`, shared.** The import half of `register_catalog_provider` moves to a private helper that both it and `_resolve_extension_catalogs` call, so the two paths accept exactly the same shapes — an object exposing `__datafusion_catalog_provider__`, a bare capsule, a `PyCatalog`, or a Python object implementing the provider interface. `PyCatalog::new` was not a substitute: it wraps unconditionally in `RustWrappedPyCatalogProvider` and never checks for a capsule, so routing the bundle path through it would have silently bypassed the FFI fast path. **Catalogs replace; tables do not.** This is the one place in the stack where two neighbouring fields take opposite rules, so it is worth being explicit. `register_catalog` returns whichever provider it displaced rather than refusing, and the default `datafusion` catalog always exists — so replacing one is the usual way a library backs a session with its own metadata, and refusing it would refuse the main use case. A duplicate *table*, by contrast, is an error, because DataFusion refuses one. Only two bundles claiming the same catalog name within a single call is refused, with both named. `table-providers.md` states the contrast where a reader meets it. **`_install_extension_catalogs` returns `()`.** Once the providers are imported there is nothing left that can fail. **The capstone test.** `test_four_libraries_install_in_one_call` installs four independently declared bundles — functions, rules, a table plus a table function, a catalog — in one `with_extensions` call, then runs a single query that reads the declared table and the declared catalog through the declared scalar function, and asserts both optimizer rules fired while planning it. That is the thing #1676 asked for, exercised across a real FFI boundary. **Skill update.** Rule 2 of `.ai/skills/ffi-capsule-protocol/SKILL.md` now carries the split this stack is built on: which components a bundle hands over unwrapped and why, pointing at `RecordingTableFunction` as the assertion rather than the description. Rule 6 gains the other convention — a new kind of component means a new resolve step returning an opaque carrier and a commit step returning `()`, never a fallible commit — along with the single table exception and a note not to add a second without the same justification. # Are there any user-facing changes? One new optional field on `SessionExtensionComponents`, defaulting to `()`. `register_catalog_provider` is refactored but behaviourally identical — same accepted shapes, same result. No hook signatures change and no wire format changes, so there is no upgrade-guide entry and no `api change` label. Taking the stack as a whole, `SessionExtensionComponents` grows from two fields to nine, all optional and all additive, and `PhysicalOptimizerRuleExportable` becomes importable from `datafusion` as well as `datafusion.context`. # Review notes **The replace-vs-refuse asymmetry is inherited, not chosen.** Both rules are DataFusion's. If you would rather `with_extensions` normalized them — refusing both, or replacing both — that is a defensible position, but it would mean the bundle path behaving differently from the `register_*` method it is meant to replace, which seems worse than the asymmetry. **What this stack does not do**, stated once so it is on the record: `with_extensions` still cannot install *everything* a library provides. A config extension has to reach `SessionConfig` before the context exists, which no bundle hook can reach — `dfx_engine` in #1721 needs exactly that, and its `build_session` will still have a step for it. **Follow-up, once #1721 and this stack have both landed.** `dfx_udfs` deliberately ships no bundle hook and carries `test_this_library_cannot_be_installed_as_a_bundle`, whose premise #1738 inverts. Whichever lands second should convert it and flip that test. Nothing in this stack touches `examples/distributed/`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
