Skip to content

Commit 872afb6

Browse files
timsaucerclaude
andcommitted
docs: give each docstring claim one canonical home
The extension docstrings and the FFI guide had grown the same six claims 3-5 times each, in wording that had already started to drift. `with_extensions` was 123 lines — the longest docstring in the package by 30 — and most of it argued a design rather than stating a contract. Routes each recurring claim to one home and leaves a one-line pointer elsewhere. Session sharing and context lifetime move onto the `SessionContext` class docstring, since they are properties of the type and that is exactly why five methods had each reworded them. The `None`-vs-`fallback` contract moves onto `SessionPlannerExportable`, since it is a return-value contract of one method. The two-phase rationale, the objects-not-capsules argument, and the codec-order argument stay in the guide, which owns the "why". The duck-type-the-session rule moves from `QueryPlannerExportable` to `LogicalExtensionCodecExportable`, which is already the designated `session` reference for that family and where the codec protocols were pointing for it anyway. It also gains the fact the old text was missing: across the protocol this argument is not always a session, so duck-typing it is not a style preference. Not every docstring shrank. `__datafusion_query_planner__` was 4 lines with no example while `set_query_planner` told callers to capture the fallback through it, so it grew to 25. Several others grew by gaining the `Args`/`Returns`/ `Raises` sections they were missing, and by trading `+SKIP` examples for runnable ones — a `SessionContext` satisfies the capsule-getter protocols, so its own exported capsule stands in for a library's without a build step. The total across these sixteen docstrings is roughly flat, at 577 lines before and 629 after; what changed is that the rationale left and the contract arrived. The 21 lines of `dataclasses.fields` rationale on `SessionExtensionComponents.__post_init__` become comments in the method body. That is the fix rather than touching `autoapi_options`: `__post_init__` is a *special* member, so dropping `private-members` would not hide it, and dropping `special-members` would delete the entire `__datafusion_*__` reference surface. `conf.py` now records why that setting is deliberately left alone, and de-duplicates the four re-exported `extensions` classes the way it already did for `DataFrame` and `SessionContext`. Adds `python/tests/test_docstrings.py`, which is what would have caught the 123-liner: a 95-line ceiling with no waiver list, a doctest-presence check over the extension protocol, and a check that a docstring naming the guide actually links it. The last two each found a real defect on first run. Also fixes what this branch made newly load-bearing in the serialization surface: `plan.py`'s single-backtick RST rendered `LogicalExtensionCodec` and friends as italics rather than links into the new API, and the `global_ctx()` fallback in `Expr.from_bytes` now silently yields a context with no extension codecs, which before this branch lost only registrations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 7c86fb0 commit 872afb6

11 files changed

Lines changed: 759 additions & 325 deletions

File tree

‎AGENTS.md‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,47 @@ Every Python function must include a docstring with usage examples.
133133
`array_sort`) only need a one-line description and a `See Also` reference to the
134134
primary function. They do not need their own examples.
135135

136+
### One canonical home per claim
137+
138+
A claim lives where the reader already is when they need it — **exactly once**.
139+
140+
- A property of one callable's arguments, return value, or errors → that
141+
callable's docstring.
142+
- A property of a *type* that several callables share → the class docstring.
143+
(Session sharing and context lifetime live on `SessionContext`, not on each
144+
of the five `with_*` methods.)
145+
- **Why** the API is shaped this way, a multi-library recipe, Rust-side code, a
146+
trade-off, or a limitation with an upstream issue → the narrative guide under
147+
`docs/source/`.
148+
149+
Everywhere else: one sentence plus one Sphinx role. A docstring may *state* a
150+
claim the guide also makes; it must not *argue* it — no "because", no "the
151+
reason is", no counter-argument.
152+
153+
**The test:** if a paragraph would survive being moved into the guide unchanged,
154+
move it. `python/tests/test_docstrings.py` enforces a length ceiling, which is
155+
the symptom this rule treats.
156+
157+
When you point at the guide, use a real `:ref:` to a specific label. Prose
158+
saying "see the extensions guide" with no role is a dead end in the rendered
159+
HTML.
160+
161+
### Examples that need a compiled extension
162+
163+
Some APIs cannot be demonstrated without a built FFI extension library, which
164+
this package does not ship. The convention is:
165+
166+
1. A **runnable** example block first, using only the wheel. A `SessionContext`
167+
satisfies the capsule-getter protocols, so its own exported capsule stands in
168+
for a real library's in a doctest.
169+
2. Then a `# doctest: +SKIP` block showing real usage, at most a few lines, with
170+
one line of prose naming the test that runs it for real.
171+
3. Every `+SKIP` block needs that mirror. See
172+
`test_with_extensions_docstring_example_still_runs` in
173+
`examples/datafusion-ffi-query-planner-example`, which parses the live
174+
docstring, drops the skip, and executes it — so a renamed method or a wrong
175+
expected output fails there.
176+
136177
## Aggregate and Window Function Documentation
137178

138179
When adding or updating an aggregate or window function, ensure the corresponding

‎docs/source/conf.py‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,11 @@
103103
# This pattern also affects html_static_path and html_extra_path.
104104
exclude_patterns = []
105105

106+
# `autoapi_options` is deliberately left at the default. In particular
107+
# `special-members` is load-bearing: it is what publishes the
108+
# `__datafusion_*__` capsule getters, which are the public protocol an
109+
# extension library implements against. Dropping it would delete that
110+
# reference surface along with `DataFrame.__init__` and the Arrow dunders.
106111
autoapi_dirs = ["../../python"]
107112
autoapi_ignore = ["*tests*"]
108113
autoapi_member_order = "groupwise"
@@ -116,6 +121,10 @@ def autoapi_skip_member_fn(app, what, name, obj, skip, options) -> bool: # noqa
116121
# Re-exports
117122
("class", "datafusion.DataFrame"),
118123
("class", "datafusion.SessionContext"),
124+
("class", "datafusion.QueryPlannerExportable"),
125+
("class", "datafusion.SessionExtensionComponents"),
126+
("class", "datafusion.SessionExtensionExportable"),
127+
("class", "datafusion.SessionPlannerExportable"),
119128
("module", "datafusion.common"),
120129
# Duplicate modules (skip module-level docs to avoid duplication)
121130
("module", "datafusion.col"),

‎docs/source/extension-guide/bundles.md‎

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,59 @@ off the supplied context, wrapping its codecs in `BundledLogicalCodec` /
7070
`BundledPhysicalCodec` so they carry declared ids, and constructing a Python
7171
{py:class}`~datafusion.SessionExtensionComponents`.
7272

73+
## A bundle you can run
74+
75+
Before wiring up a cdylib, it is worth seeing the protocol work end to end in
76+
pure Python. This runs against the plain wheel — the codec here re-exports the
77+
host session's own capsule, where a real one would return its library's:
78+
79+
```python
80+
from datafusion import SessionContext, SessionExtensionComponents
81+
82+
83+
class Codec:
84+
"""Wraps a capsule so it carries an id. A Rust library ships this shape."""
85+
86+
def __init__(self, codec_id, capsule):
87+
self.__datafusion_codec_id__ = codec_id
88+
self._capsule = capsule
89+
90+
def __datafusion_logical_extension_codec__(self, session=None):
91+
return self._capsule
92+
93+
94+
class Bundle:
95+
def __init__(self, codec_id):
96+
self.codec_id = codec_id
97+
98+
def __datafusion_session_extension__(self, ctx):
99+
# Fresh components on every call, bound to the `ctx` handed in.
100+
# Never cache these, and never retain `ctx`.
101+
return SessionExtensionComponents(
102+
logical_extension_codecs=(
103+
Codec(self.codec_id, ctx.__datafusion_logical_extension_codec__()),
104+
)
105+
)
106+
107+
108+
ctx = SessionContext().with_extensions(Bundle("tables.v1"), Bundle("engine.v1"))
109+
ctx.logical_extension_codec_ids()
110+
# ['tables.v1', 'engine.v1']
111+
```
112+
113+
Three things this makes observable, each pinned by a test in
114+
`python/tests/test_context.py`:
115+
116+
- **Ids accumulate in bundle order.** Decoding does not depend on that order;
117+
only encoding does. See {ref}`extension_codec_order`.
118+
- **Two bundles claiming one id are refused**, with a `ValueError` naming the
119+
id — not resolved by position, since a positional id would break stored plans
120+
the first time a bundle reordered what it returns.
121+
- **A hook that raises leaves the receiving session untouched.** Add a bundle
122+
whose hook raises and the source context's
123+
{py:meth}`~datafusion.SessionContext.logical_extension_codec_ids` is still
124+
empty afterwards.
125+
73126
(extension_bundles_two_phases)=
74127

75128
## Two phases, because codecs and planners compose differently

‎examples/datafusion-ffi-query-planner-example/python/tests/_test_three_library_query_planner.py‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1386,17 +1386,24 @@ def test_with_extensions_docstring_example_still_runs():
13861386
statements are parsed out of the live docstring, the skip is dropped, and
13871387
each one is executed and its output compared.
13881388
1389+
Only the ``+SKIP`` statements are taken. The docstring also carries a
1390+
runnable example above them, which the main suite already executes under
1391+
``--doctest-modules``; running it again here would need its own namespace
1392+
and prove nothing.
1393+
13891394
Only names are redirected: ``my_extension`` resolves to the bundle above,
13901395
and ``SessionContext`` supplies the config this library's planner reads.
13911396
A renamed method, a changed signature, or a wrong expected output in the
13921397
docstring fails here.
13931398
"""
1394-
examples = doctest.DocTestParser().get_examples(
1395-
inspect.getdoc(SessionContext.with_extensions)
1396-
)
1397-
assert examples, "with_extensions docstring has no examples to check"
1398-
for example in examples:
1399-
example.options.pop(doctest.SKIP, None)
1399+
examples = [
1400+
example
1401+
for example in doctest.DocTestParser().get_examples(
1402+
inspect.getdoc(SessionContext.with_extensions)
1403+
)
1404+
if example.options.pop(doctest.SKIP, False)
1405+
]
1406+
assert examples, "with_extensions docstring has no skipped examples to check"
14001407

14011408
module = types.ModuleType("my_extension")
14021409
module.DistributedEngineExtension = _DocstringExampleExtension

0 commit comments

Comments
 (0)