Conversation
This was referenced Sep 15, 2026
timsaucer
added this pull request to stack #1742
September 15, 2026 14:28
timsaucer
marked this pull request as draft
September 15, 2026 14:28
`SessionExtensionComponents.physical_optimizer_rules` completes the group of components whose capsule getter takes no argument, so a library shipping a rule alongside anything else no longer asks the caller for a separate `add_physical_optimizer_rule` call. Rules are the one kind with no collision rule: they accumulate rather than replace, so two bundles contributing one each is the normal case and there is nothing to refuse. Installing them splits across the two new private primitives `_resolve_extension_physical_optimizer_rules` and `_install_extension_physical_optimizer_rules`, keeping the commit step infallible: the capsules are imported during resolution, so a rule that fails to import cannot leave the session with a planner already bound. All the rules in one call go on in a single `SessionState` rebuild. `add_physical_optimizer_rule` rebuilds per call, which for a bundle with several would clone the whole state that many times and leave the earlier ones installed if a later one failed. The session id is carried across the rebuild for the same reason that method carries it. `PhysicalOptimizerRuleExportable` moves from `datafusion.context` to `datafusion.extensions`, alongside the rest of the protocol family, and is now exported from the package root. It stays importable from `datafusion.context`. `MyRuleExtension` in `datafusion-ffi-example` declares two rules, which is what makes accumulation observable — each carries its own counter and both fire. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
timsaucer
force-pushed
the
feat/bundle-optimizer-rules
branch
from
September 15, 2026 20:04
74edaff to
3a24768
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.
Which issue does this PR close?
Part 2 of 4 toward #1676.
Rationale for this change
#1738 added the function fields and, with them, the rule that installing a component splits into a fallible resolve step and an infallible commit step. Physical optimizer rules are the other half of the group that rule was written for: like the three function getters,
__datafusion_physical_optimizer_rule__takes no argument, so resolution needs nothing from the session and has no ordering constraint. Landing them here finishes that group before the stack moves on to the components that do need the session.There is a second reason that is independent of bundles.
add_physical_optimizer_ruledoes a fullSessionStateBuilder::new_from_existing(guard.clone()).build()per call. A library contributing three rules clones the entire session state three times, and a failure on the third leaves the first two installed. Batching removes both.What changes are included in this PR?
SessionExtensionComponents.physical_optimizer_rules. Objects exposing__datafusion_physical_optimizer_rule__, installed in declaration order.Rules never collide. This is the one component kind with no collision rule at all. Two functions claiming a name is a
ValueErrorbecause a function registry has no fall-through; rules accumulate, so two libraries each contributing one is the normal case and there is nothing to refuse.extension-guide/other-components.mdalready said "rules accumulate where planners nest" — this makes the bundle path agree with it.Two new private Rust primitives, split to keep the commit infallible.
_resolve_extension_physical_optimizer_rulesimports every capsule and writes nothing;_install_extension_physical_optimizer_rulesapplies them all in oneSessionStaterebuild and returns()rather than aResult, because by then there is nothing left that can fail. This is the contract #1738 stated at the commit boundary, and it matters concretely here: a rule that failed to import after the planner was bound would leave the session half-installed with nothing to roll back to, since the returned handle and the receiver are one session.The imported rules travel from resolve to commit in
PyPhysicalOptimizerRules, a pyclass deliberately not added to the module —with_extensionsis its only producer and only consumer. Both primitives are added to the private-method allowlist intest_wrapper_coverage.py.The session id is carried across the rebuild for the same reason
add_physical_optimizer_rulecarries it: the builder mints a fresh one, and losing it leavessession_id()disagreeing with everyTaskContextthe session has already handed out — which is what a codec's decode callbacks resolve against.PhysicalOptimizerRuleExportablemoves todatafusion.extensions. It was the one member of the protocol family still living indatafusion.context, absent fromdatafusion.__all__, and with no example. It is now beside its siblings, exported from the package root, and documented with a runnable block showing the refusal a non-capsule gets plus a+SKIPblock naming the test that runs the real thing. It remains importable fromdatafusion.context, so this is purely additive.Real-FFI coverage.
MyRuleExtensiondeclares twoMyPhysicalOptimizerRuleinstances, which is what makes accumulation observable — each carries its own call counter and both fire on the next query. The new tests cover that, the session id surviving the rebuild, functions and rules arriving from two different bundles in one call, and a failure after the hooks leaving neither rule installed. That last one asserts on the counters rather than on a registry, since an installed rule is invisible tosession_id()and toudf().Are there any user-facing changes?
One new optional field on
SessionExtensionComponents, defaulting to().PhysicalOptimizerRuleExportableis now importable fromdatafusionas well asdatafusion.context. Nothing is removed or renamed, no hook signatures change, so there is no upgrade-guide entry and noapi changelabel.Worth naming:
add_physical_optimizer_rulestill rebuilds once per call. Only the bundle path batches. Changing the public method's behaviour is not needed for this issue and would be a separate judgement about whether anyone depends on the per-call rebuild.Review notes
The infallible-commit split is the part to push on. If you think importing the capsules could just as well happen inside the commit call, the case against it is that
_install_extension_plannerruns first: aResultreturned after that point is a partially-installed session, and the only reason it would ever be one is convenience. Making the commit method return()rather thanPyDataFusionResult<()>is the enforcement — it cannot silently grow a failure path later.PyPhysicalOptimizerRulesis not exported. If you would rather it were a plain opaque capsule than a pyclass, say so; the pyclass was chosen because it carriesVec<Arc<dyn PhysicalOptimizerRule>>without a second unsafe boundary.🤖 Generated with Claude Code