Skip to content

Commit 07792ad

Browse files
timsaucerclaude
andcommitted
Make the planner-hook tests assert what their names claim
`test_with_extensions_threads_the_planner_through_in_order` never checked an order: it asserted each hook recorded one fallback and that the second was not None, both of which hold for a host that ran the hooks backwards. `test_with_extensions_skips_a_planner_hook_returning_none` asserted only that downstream ran, while its comment claimed the skipped hook had not become downstream's fallback. `_PlannerExtension` now takes an optional shared list the hooks append themselves to, so order is observable. The threading test asserts that list, plus that the second hook's fallback is not the object the first was handed -- the host re-exports every return value before passing it on. A capsule is opaque from Python, so that cannot separate a re-export of the first planner from a fresh read of the session's; the comment says so and points at the FFI suite's `test_with_extensions_nests_planners_in_argument_order`, which pins the nesting by asserting the outer planner delegated. The skip test records the skipped hook's fallback too, and asserts both hooks ran, that downstream was handed a different capsule, and that the resulting context still queries. Each new assertion was mutation-tested: reversing the planner loop, dropping the `if supplied is None: continue`, and replacing the re-export with a straight pass-through each fail exactly one of these tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent d13787b commit 07792ad

1 file changed

Lines changed: 47 additions & 13 deletions

File tree

python/tests/test_context.py

Lines changed: 47 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -942,11 +942,15 @@ class _PlannerExtension:
942942
foreign planner — but nothing here depends on that either way.
943943
"""
944944

945-
def __init__(self):
945+
def __init__(self, calls=None):
946946
self.fallbacks = []
947947
self.planner_ctx = None
948+
# Shared list the hooks append themselves to, so a test can assert the
949+
# order they ran in rather than only that each ran.
950+
self.calls = [] if calls is None else calls
948951

949952
def __datafusion_session_planner__(self, ctx, fallback):
953+
self.calls.append(self)
950954
self.planner_ctx = ctx
951955
self.fallbacks.append(fallback)
952956
return fallback
@@ -1053,15 +1057,25 @@ def test_with_extensions_threads_the_planner_through_in_order(ctx):
10531057
Planners nest rather than chain, so the host hands each bundle the planner
10541058
built so far. Argument order is nesting order, last one outermost.
10551059
"""
1056-
first, second = _PlannerExtension(), _PlannerExtension()
1060+
calls = []
1061+
first, second = _PlannerExtension(calls), _PlannerExtension(calls)
10571062
ctx.with_extensions(first, second)
10581063

1059-
assert len(first.fallbacks) == 1
1060-
assert len(second.fallbacks) == 1
1061-
# `first` returned its fallback unchanged, and the host normalizes each
1062-
# hook's return value before passing it on, so `second` sees a capsule
1063-
# standing for the same planner rather than the session's original.
1064-
assert second.fallbacks[0] is not None
1064+
# Argument order, once each. Nothing else pins the order: both hooks
1065+
# return capsules, and a host that ran them backwards would still leave
1066+
# each with one fallback recorded.
1067+
assert calls == [first, second]
1068+
1069+
# `first` returned its fallback unchanged, but the host re-exports every
1070+
# hook's return value before handing it on, so `second` receives a capsule
1071+
# of its own rather than the object `first` was handed.
1072+
assert second.fallbacks[0] is not first.fallbacks[0]
1073+
1074+
# That is as far as pure Python reaches: a capsule is opaque, so this
1075+
# cannot tell a re-export of `first`'s planner from a fresh read of the
1076+
# session's. `test_with_extensions_nests_planners_in_argument_order` in
1077+
# examples/datafusion-ffi-query-planner-example is what pins the nesting,
1078+
# by asserting the outer planner delegated to the inner one.
10651079

10661080

10671081
def test_with_extensions_planner_hook_sees_the_new_handle(ctx):
@@ -1079,18 +1093,38 @@ def test_with_extensions_planner_hook_sees_the_new_handle(ctx):
10791093

10801094

10811095
def test_with_extensions_skips_a_planner_hook_returning_none(ctx):
1082-
"""Returning ``None`` contributes no planner and keeps the fallback."""
1096+
"""Returning ``None`` contributes no planner and keeps the fallback.
1097+
1098+
The skip is what lets the call succeed at all: a host that treated the
1099+
``None`` as a contribution would hand it to the export step and fail with
1100+
``'None' is not an instance of 'PyCapsule'`` before ``downstream`` ran.
1101+
"""
10831102

10841103
class NoPlanner:
1104+
def __init__(self):
1105+
self.fallbacks = []
1106+
10851107
def __datafusion_session_planner__(self, ctx, fallback):
1086-
return None
1108+
self.fallbacks.append(fallback)
1109+
# Spelled out rather than left to fall off the end: `None` is the
1110+
# protocol's "contribute no planner", which is what this test is
1111+
# about, and an implicit one would read as an oversight.
1112+
return None # noqa: RET501, PLR1711
10871113

1114+
skipped = NoPlanner()
10881115
downstream = _PlannerExtension()
1089-
ctx.with_extensions(NoPlanner(), downstream)
1116+
result = ctx.with_extensions(skipped, downstream)
10901117

1091-
# The skipped hook did not become `downstream`'s fallback; it got the
1092-
# session's own planner instead.
1118+
# Both hooks ran, and `downstream` was handed a planner rather than the
1119+
# `None` in front of it. It is a fresh read of the session's planner, not
1120+
# the object `skipped` was given, so a host that fell back by reusing the
1121+
# previous hook's *input* is ruled out too.
1122+
assert len(skipped.fallbacks) == 1
10931123
assert len(downstream.fallbacks) == 1
1124+
assert downstream.fallbacks[0] is not skipped.fallbacks[0]
1125+
1126+
batches = result.sql("SELECT 1 AS value").collect()
1127+
assert batches[0].column(0) == pa.array([1])
10941128

10951129

10961130
def test_with_extensions_rejects_bad_codec_capsule(ctx):

0 commit comments

Comments
 (0)