Skip to content

Commit 2ea2379

Browse files
timsaucerclaude
andcommitted
refactor: collapse the two collision messages into one
The branch on `claimed_at == position` produced two messages so each could name only the remedy its reader could apply. One message naming both argument positions carries the same information: two different positions means two bundles and is the caller's to resolve, one position named twice means a single bundle claimed the name twice and is its author's. The reader works that out from the numbers, which were already in the message. Position-keying still matters and keeps its comment -- object identity would read one extension passed twice as a bundle colliding with itself, and offer a rename of someone else's function that the caller cannot make. `test_with_extensions_rejects_one_extension_passed_twice` loses its assertion that the message never says "rename", which is the behaviour being removed; it and its sibling now pin the argument positions instead, which is what tells the two cases apart. Docstrings and both guides drop the "two extensions" framing -- one extension declaring a name twice was always refused too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent f11a99f commit 2ea2379

5 files changed

Lines changed: 44 additions & 50 deletions

File tree

docs/source/extension-guide/bundles.md

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -290,27 +290,27 @@ a capsule.
290290

291291
## Two bundles claiming one name
292292

293-
Two extensions in one call may not declare a function of the same kind under
294-
the same name. Doing so raises:
293+
One `with_extensions` call may not declare a function of the same kind under
294+
the same name twice. Doing so raises:
295295

296296
```text
297-
ValueError: Two extensions declare a scalar function named 'normalize':
298-
argument 0 (...) and argument 1 (...). ...
297+
ValueError: A scalar function named 'normalize' is declared twice: argument 0
298+
(<lib_a.Extension>) and argument 1 (<lib_b.Extension>). ...
299299
```
300300

301301
Codecs get away with sharing a chain because a payload carries the id of the
302302
codec that wrote it, so decode routes to the right one. A function registry has
303303
no such fall-through — one name holds one function — so the second registration
304304
would quietly replace the first. The call refuses instead.
305305

306-
Which argument each claim came from is part of the message because it is what
307-
picks the remedy. Two arguments colliding is the caller's to resolve, by
308-
installing the two on separate sessions or by dropping a repeat; renaming is
309-
not something a caller can do. One argument declaring a name twice is the
310-
bundle author's own bug, and gets a different message saying so. Collisions are
311-
keyed on position rather than on object identity, so passing one extension
312-
twice reads as the caller's duplicate that it is, rather than as a bundle
313-
colliding with itself.
306+
The argument each claim came from is in the message because it is what picks
307+
the remedy, and the reader works it out from the two positions. Two different
308+
positions means two bundles, which only the caller can resolve — by dropping a
309+
repeat, or by installing them on separate sessions. One position named twice
310+
means a single bundle claimed the name twice, which is its author's own bug and
311+
the one case a rename fixes. Collisions are keyed on position rather than on
312+
object identity so that passing one extension twice reads as the caller's
313+
duplicate that it is, rather than as a bundle colliding with itself.
314314

315315
Three cases this does *not* catch:
316316

docs/source/user-guide/extensions.md

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,8 @@ same kind under the same name, the call raises a `ValueError` naming both,
106106
rather than letting one silently replace the other:
107107

108108
```text
109-
ValueError: Two extensions declare a scalar function named 'normalize': ...
109+
ValueError: A scalar function named 'normalize' is declared twice: argument 0
110+
(<lib_a.Extension>) and argument 1 (<lib_b.Extension>). ...
110111
```
111112

112113
You cannot rename another library's function from your own code, so the fix is
@@ -121,6 +122,8 @@ supported thing for a library to do. See {ref}`extension_bundles_collisions`.
121122
Check the argument positions the message names before you go looking for a
122123
second library. Passing one extension twice collides with itself, and an
123124
extension list assembled from a plugin registry is the usual way that happens.
125+
If both positions are the *same* number, only one library is involved and it
126+
declared the name twice — nothing on your side fixes that, so report it.
124127

125128
**Functions outlive the handle you installed them on.** `with_extensions`
126129
returns a new context, and its codecs belong to that context alone — but

python/datafusion/context.py

Lines changed: 13 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -296,25 +296,19 @@ def _resolve_declared_functions(
296296
raise TypeError(msg)
297297
name = wrapped.name
298298
if name in claimed:
299-
# Keyed on position, not object identity, so each message names
300-
# the remedy its reader actually has — see
299+
# Keyed on position, not object identity: one argument listed
300+
# twice is what tells the reader a single bundle claimed the name
301+
# twice, which is the only case a rename can fix. See
301302
# `extension_bundles_collisions` in the extension guide.
302303
claimed_at, claimed_by = claimed[name]
303-
if claimed_at == position:
304-
msg = (
305-
f"{extension!r} declares two {label}s named {name!r}. "
306-
"Registrations have no fall-through, so the second would "
307-
"silently replace the first; rename one of them."
308-
)
309-
else:
310-
msg = (
311-
f"Two extensions declare a {label} named {name!r}: "
312-
f"argument {claimed_at} ({claimed_by!r}) and argument "
313-
f"{position} ({extension!r}). Registrations have no "
314-
"fall-through, so one would silently replace the other; "
315-
"install them on separate sessions, or drop the repeat if "
316-
"one extension was passed twice."
317-
)
304+
msg = (
305+
f"A {label} named {name!r} is declared twice: argument "
306+
f"{claimed_at} ({claimed_by!r}) and argument {position} "
307+
f"({extension!r}). Registrations have no fall-through, so one "
308+
"would silently replace the other. Rename one if both came "
309+
"from an extension you own, drop the repeat if one extension "
310+
"was passed twice, or install them on separate sessions."
311+
)
318312
raise ValueError(msg)
319313
claimed[name] = (position, extension)
320314
resolved.append(wrapped)
@@ -2103,8 +2097,8 @@ def with_extensions(
21032097
``PyCapsule`` rather than an object exposing the getter, or if
21042098
a declared function is neither a wrapper nor exposes its
21052099
capsule getter.
2106-
ValueError: If two codecs claim the same id, if two extensions
2107-
declare a function of one kind under the same name, or if a
2100+
ValueError: If two codecs claim the same id, if one call declares a
2101+
function of one kind under the same name twice, or if a
21082102
getter returns a capsule of the wrong kind. See
21092103
:py:meth:`with_logical_extension_codec` for how ids are
21102104
assigned.

python/datafusion/extensions.py

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -233,10 +233,10 @@ class SessionExtensionComponents:
233233
:py:func:`~datafusion.udf` on the way in. The registered name comes from
234234
the function itself, not from this field.
235235
236-
Two extensions in one
237-
:py:meth:`~datafusion.context.SessionContext.with_extensions` call may not
238-
declare the same name; shadowing a function the session already has is
239-
allowed. See :ref:`extension_bundles_collisions`.
236+
One :py:meth:`~datafusion.context.SessionContext.with_extensions` call may
237+
not declare a name twice, whether the two claims come from one extension or
238+
two; shadowing a function the session already has is allowed. See
239+
:ref:`extension_bundles_collisions`.
240240
"""
241241

242242
udafs: tuple[AggregateUDF | AggregateUDFExportable, ...] = ()

python/tests/test_context.py

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1570,34 +1570,31 @@ def test_with_extensions_rejects_a_name_two_extensions_claim(
15701570
def test_with_extensions_rejects_one_extension_passed_twice(ctx):
15711571
"""A bundle object listed twice is the caller's duplicate, not a naming bug.
15721572
1573-
The remedy has to match the mistake, and nothing the bundle author renames
1574-
helps here — both claims come from the one declaration. Collisions are
1575-
therefore keyed on argument position rather than on object identity, which
1576-
would read a repeat as a bundle colliding with itself and offer a rename
1577-
that cannot be made.
1573+
Two argument positions is what says so, and it is the reason collisions are
1574+
keyed on position rather than on object identity: identity would read a
1575+
repeat as one bundle colliding with itself, pointing the reader at a rename
1576+
of someone else's function that they cannot make.
15781577
"""
15791578
extension = _FunctionExtension(udfs=(_doubler(),))
15801579

1581-
with pytest.raises(ValueError, match=r"argument 0 .* and argument 1 ") as excinfo:
1580+
with pytest.raises(ValueError, match=r"argument 0 .* and argument 1 "):
15821581
ctx.with_extensions(extension, extension)
15831582

1584-
assert "rename" not in str(excinfo.value)
15851583
with pytest.raises(KeyError):
15861584
ctx.udf("double")
15871585

15881586

15891587
def test_with_extensions_rejects_a_name_one_extension_claims_twice(ctx):
1590-
"""A bundle colliding with itself is its own bug, not a clash of libraries.
1588+
"""A bundle colliding with itself is caught by the same check.
15911589
1592-
Separated from the two-extension case because the remedy differs: a bundle
1593-
author can rename their own function, and a caller cannot rename someone
1594-
else's.
1590+
One argument position named twice is what distinguishes it, and it is the
1591+
case the message's rename remedy is for — the only reader who can rename a
1592+
function is the one who declared both of them.
15951593
"""
1596-
with pytest.raises(
1597-
ValueError, match=r"declares two scalar functions named 'double'"
1598-
):
1594+
with pytest.raises(ValueError, match=r"argument 0 .* and argument 0 ") as excinfo:
15991595
ctx.with_extensions(_FunctionExtension(udfs=(_doubler(), _doubler())))
16001596

1597+
assert "scalar function named 'double'" in str(excinfo.value)
16011598
with pytest.raises(KeyError):
16021599
ctx.udf("double")
16031600

0 commit comments

Comments
 (0)