Conversation
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
This was referenced Sep 15, 2026
timsaucer
force-pushed
the
feat/bundle-tables
branch
2 times, most recently
from
September 16, 2026 15:31
413b8c2 to
cad7e55
Compare
`SessionExtensionComponents` gains `udtfs` and `table_providers`, both as `(name, value)` pairs. Neither carries a name of its own the way a scalar function's capsule does, and both getters take the session — which is what makes them different from everything the stack has added so far. Because they take the session, the host resolves them against the handle carrying the completed codec chains rather than against the context the components hook received. A bundle wrapping one itself would bind it to a chain missing every library in the call, including its own, and the failure would not surface until a decode somewhere else. So a bundle hands over the unwrapped value and lets the host wrap it. `RecordingTableFunction` in the example crate records the codec ids it was handed, which turns that claim into an assertion rather than a paragraph. Tables do not shadow. DataFusion refuses a duplicate table registration rather than replacing it, so a declared name already on the session is an error too, not just one two bundles both claim. Both are caught while resolving, alongside resolving the destination schema, so a bad name costs nothing. `_resolve_extension_tables` and `_install_extension_tables` keep the same split as the rules, with one honest exception: the insert goes through a `SchemaProvider`, and a foreign one can still refuse what it reported as free. Tables are therefore committed first, so nothing else has been written when that happens. The guide says so rather than claiming a guarantee that does not hold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A junk table value fell through to the pyarrow Dataset fallback, whose error names neither the table nor the bundle. The declared name is unique within the call, so wrapping the import error with it points at one declaration — the same repair the rules got, adapted to a component whose value has four legal shapes and so cannot be pre-checked in Python. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Same reasoning as PhysicalOptimizerRules gaining frozen on the base branch: nothing mutates it between resolve and commit, so there is no reason to pay for the runtime borrow flag a mutable pyclass carries. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The resolve and commit steps in ffi-internals.md predate declared tables. Tables resolve alongside everything else, but their insert goes through a SchemaProvider, and a foreign one can still refuse what it reported as free — the one honest exception to "step 4 cannot raise", already stated in the bundle guide, now stated where the rule for the next field lives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`_reject_repeated_names` compares the declared strings, and the `table_exist` check in `_resolve_extension_tables` runs against a schema nothing has been written to yet. Two declarations that resolve to one table but differ as strings passed both: `TableReference::from` lowercases a name and splits it, and the default catalog and schema fill in the rest, so `Events`, `events` and `public.events` are one destination under three spellings. Both then resolved, and the duplicate surfaced from the insert during the commit with the first table already registered -- the part-applied outcome the two-phase split exists to rule out, reachable through the default in-memory schema provider rather than only through a foreign one. Resolve each name to a `ResolvedTableReference` and key the check on that. The Python pass stays: only the resolver knows two spellings are one table, and only Python knows which argument each declaration came from, which is what picks the remedy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`__post_init__` catches `udfs=fn` written for `udfs=(fn,)` so the error
lands in the extension library's own frame rather than deep inside
`with_extensions`. The pair-shaped fields have the same mistake one level
in — `udtfs=("expand", fn)` is a two-element tuple, so it normalized
without complaint and surfaced later as `'function' object is not
subscriptable`, naming neither the field nor the bundle.
Mark those fields in the field metadata and check each item is a
`(str, value)` pair, normalizing it to a tuple like everything else here.
A str item is rejected before unpacking: a two-letter name would
otherwise unpack into two characters and pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four review nits, all documentation. The no-shadowing rule was credited to DataFusion. It is not DataFusion's: a `SchemaProvider` decides for itself whether a duplicate replaces or refuses, and only the in-memory one a session starts with refuses. `with_extensions` does not ask — it settles the question during resolve, which is what buys one rule for every destination and a refusal while a failure is still free, and which makes a bundle stricter than `register_table` against a provider that would have replaced. Said once in the collisions section and pointed at from the field docstring, the Rust resolver and the test that pins it. `udtfs` pointed at the two-phase section for a claim whose canonical home is the binding section this PR added; `functions.md` and `table-providers.md` already point there. The `Raises:` entry for a declared table did not mention two declarations resolving to one, and a rewrap had left "registering a" alone on a line. The user guide now says what a caller meets: a bundle cannot take a table name they already used. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
timsaucer
force-pushed
the
feat/bundle-tables
branch
from
September 16, 2026 18:01
a6c147f to
300e321
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 3 of 4 toward #1676.
Rationale for this change
#1738 and #1739 covered every component whose capsule getter takes no argument. Tables and table functions are the other kind — their getters take the session — and that difference is the whole of this PR. It is also the second demonstrated gap from #1721:
build_sessionregisters tables as a separate step 4, afterwith_extensions.What changes are included in this PR?
udtfsandtable_providers, both as(name, value)pairs. Neither carries a name of its own the way a scalar function's capsule does, so the name is given alongside the value.table_providersaccepts anythingregister_tabledoes; names may be qualified.The host resolves them, and the bundle must not.
__datafusion_table_function__and__datafusion_table_provider__are handed the session, and__datafusion_table_function__pulls the host's logical codec straight off it. The context a components hook receives has none of the call's codecs installed yet, so a bundle that wrapped its ownTableFunctionwould capture a chain missing every library in the call — including its own — and nothing would go wrong until a decode in another process. So bundles hand over the unwrapped value and the host wraps it against the finished handle, in the resolve step.That is a claim worth asserting rather than describing, so
RecordingTableFunctionin the example crate records the codec ids of whatever session it is resolved against, andtest_a_declared_table_function_sees_the_finished_codec_chaininstalls it alongside a second bundle that contributes a codec. The recorded ids match the finished chain and are non-empty; resolving against the hook's context would record[].Tables do not shadow. DataFusion refuses a duplicate table registration rather than replacing it, so unlike a function, a declared table name that is already on the session is an error — not only one that two bundles both claim. Both cases are caught during resolution, along with resolving the destination schema, so a bad or qualified-but-unknown name costs nothing and the rest of the call is left unwritten.
_resolve_extension_tables, with the commit as a_commit_extensionsparameter, following the split #1739 established, with one exception I want to be explicit about rather than paper over: the insert goes through aSchemaProvider, and a foreign one can still refuse a registration it reported as available. That is the one place inwith_extensionsthat can leave a call part-applied. Tables are therefore committed first, so when it happens no planner has been bound and no function registered behind it.extension-guide/bundles.mdstates this as an exception to the infallible-commit rule rather than claiming a guarantee that does not hold.Docs.
bundles.mdgains anextension_bundles_bindingsection splitting components by what their getter asks for, which is the rule the rest of the stack is built on; the transaction section gains the table exception; the collision section gains the no-shadowing rule.table-providers.mdandfunctions.mdeach gain the bundle form and a pointer to why the value is handed over unwrapped.user-guide/extensions.mdno longer claims tables always register directly.Are there any user-facing changes?
Two new optional fields on
SessionExtensionComponents, defaulting to(). No existing behaviour changes:register_tableandregister_udtfare untouched, and the new resolution path is only reachable throughwith_extensions. No upgrade-guide entry and noapi changelabel.Review notes
The pair shape is worth arguing about.
udfsaccepts either a wrapper or a raw exportable;udtfsaccepts only the raw value plus a name. That is not an inconsistency for its own sake — aTableFunctioncan only be built by calling the getter with some session, and a bundle does not have the right one. Accepting a pre-built one would mean accepting one bound to the wrong chains. If you would rather it accepted both and documented the hazard, say so.table_existthen insert is a check-then-act. Under the GIL, within a singlewith_extensionscall, nothing else is registering concurrently. The alternative is to drop the check and let the duplicate surface from the insert, which would report the same problem after the call had already written the tables ahead of it.🤖 Generated with Claude Code