Skip to content

Commit f448a72

Browse files
timsaucerclaude
andcommitted
fix: skip the planner rebind when inlining is unchanged
`with_python_udf_inlining` rebuilds the handle's codecs and rebinds the session's planner to them, which is state shared with every other handle on that session. Asking for the setting a context already has changes nothing, so it should not pay that side effect: a defensive no-op toggle on `ctx` otherwise drags the planner back onto `ctx`'s codecs and silently undoes a codec installed through another handle. Returning the existing codecs is observationally equivalent to the rebuild otherwise -- it wraps the same inner codec in a fresh `Python*Codec` -- so the guard is only visible through that side effect. The new test fails without it with `assert 0 > 0`. Also pins the divergence the rebind creates. The planner carries the codecs of whichever handle installed it last; every other path on a context uses that context's own codec field. Those can be different handles, and then `Expr.to_bytes(ctx)` and `ctx.sql(...)` encode with different codecs on the same `ctx`. Stated as a rule in the FFI guide rather than left implicit in the description of the mechanism. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 2425559 commit f448a72

3 files changed

Lines changed: 121 additions & 2 deletions

File tree

crates/core/src/context.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1508,6 +1508,23 @@ impl PySessionContext {
15081508
}
15091509

15101510
pub fn with_python_udf_inlining(&self, enabled: bool) -> Self {
1511+
// Rebinding the session's planner is a side effect on state shared with
1512+
// every other handle, so do not pay it for a call that changes nothing.
1513+
// A defensive `with_python_udf_inlining(enabled=True)` on a context that
1514+
// already inlines would otherwise rebind the session's planner to this
1515+
// handle's codecs, and callers routinely discard the result. Returning
1516+
// the codecs as-is is observationally equivalent to the rebuild below,
1517+
// which wraps the same inner codec in a fresh `Python*Codec`.
1518+
if self.logical_codec.python_udf_inlining() == enabled
1519+
&& self.physical_codec.python_udf_inlining() == enabled
1520+
{
1521+
return Self {
1522+
ctx: Arc::clone(&self.ctx),
1523+
logical_codec: Arc::clone(&self.logical_codec),
1524+
physical_codec: Arc::clone(&self.physical_codec),
1525+
};
1526+
}
1527+
15111528
let logical_codec = Arc::new(
15121529
PythonLogicalCodec::new(Arc::clone(self.logical_codec.inner()))
15131530
.with_python_udf_inlining(enabled),

docs/source/contributor-guide/ffi.md

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -351,7 +351,29 @@ that planner against the new codec for the same reason: there is one planner, an
351351
to carry the codecs currently in force. This happens on the shared session, so it takes
352352
effect even if the returned context is discarded — `ctx.with_python_udf_inlining(...)`
353353
whose result is thrown away still leaves the session's planner carrying the codecs of
354-
that discarded handle.
354+
that discarded handle. A call that changes nothing is exempt: asking for the inlining
355+
setting a context already has returns a handle without touching the session.
356+
357+
The rule that falls out of this is worth stating on its own, because it is the one thing
358+
that surprises people:
359+
360+
> The session's query planner carries the codecs of the handle that most recently
361+
> installed one. Every other path — `Expr.to_bytes(ctx)`, `ExecutionPlan.to_bytes(ctx)`,
362+
> registering a provider — uses the codecs of the handle you call it on.
363+
364+
Those can be different handles, and then one session has two codecs in effect at once:
365+
366+
```python
367+
ctx = ctx.with_logical_extension_codec(codec_a)
368+
ctx.set_query_planner(planner)
369+
ctx.with_logical_extension_codec(codec_b) # discarded
370+
371+
Expr.to_bytes(expr, ctx) # encodes with codec_a -- ctx's own field
372+
ctx.sql(...).collect() # plans with codec_b -- installed via the discarded handle
373+
```
374+
375+
Chaining `ctx = ctx.with_...(...)`, as the example below does, keeps the two in step.
376+
`test_the_planner_and_the_handle_can_hold_different_codecs` pins the divergence.
355377

356378
```python
357379
ctx = SessionContext(config)

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

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,9 @@
1919

2020
import gc
2121

22+
import pyarrow as pa
2223
import pytest
23-
from datafusion import SessionConfig, SessionContext, udf
24+
from datafusion import Expr, SessionConfig, SessionContext, col, udf
2425
from datafusion_ffi_example import (
2526
IsNullUDF,
2627
MyCatalogProvider,
@@ -535,6 +536,85 @@ def test_a_discarded_derived_context_still_rebinds_the_planner():
535536
assert later.table_provider_encode_calls() > 0
536537

537538

539+
def test_the_planner_and_the_handle_can_hold_different_codecs():
540+
"""One session, two codecs in effect, depending on the path taken.
541+
542+
The planner is session state and carries whichever codecs installed it
543+
last -- here, ones that arrived through a handle that was discarded.
544+
Everything else on a context uses that context's own codec field, which
545+
the discarded handle never touched. So `Expr.to_bytes(ctx)` and
546+
`ctx.sql(...)` encode with different codecs on the same `ctx`.
547+
548+
Chaining ``ctx = ctx.with_...(...)`` keeps the two in step; this pins what
549+
happens when they are allowed to diverge.
550+
551+
Inlining has to be off for the assertion to say anything: with it on, a
552+
Python UDF is encoded inline by ``PythonLogicalCodec`` and never reaches
553+
the installed codec's ``try_encode_udf``.
554+
"""
555+
config = SessionConfig().with_extension(MyPlannerConfig(max_rows=3))
556+
handle_codec = MyLogicalExtensionCodec()
557+
ctx = SessionContext(config).with_python_udf_inlining(enabled=False)
558+
ctx = ctx.with_logical_extension_codec(handle_codec)
559+
ctx = ctx.with_physical_extension_codec(MyPhysicalExtensionCodec())
560+
ctx.register_table("numbers", MyTableProvider(1, 6, 1))
561+
ctx.set_query_planner(MyQueryPlanner())
562+
563+
planner_codec = MyLogicalExtensionCodec()
564+
# Discarded, but the planner keeps its codec.
565+
ctx.with_logical_extension_codec(planner_codec)
566+
gc.collect()
567+
568+
identity = udf(
569+
lambda arr: arr,
570+
[pa.int64()],
571+
pa.int64(),
572+
volatility="immutable",
573+
name="identity_i64",
574+
)
575+
ctx.register_udf(identity)
576+
Expr.to_bytes(identity(col("A")), ctx)
577+
578+
# Serializing through `ctx` uses `ctx`'s own codec field.
579+
assert handle_codec.encode_udf_calls() > 0
580+
assert planner_codec.encode_udf_calls() == 0
581+
582+
ctx.sql('SELECT "A" FROM numbers ORDER BY "A"').collect()
583+
584+
# Planning through the same `ctx` uses the codec the planner was rebound to.
585+
assert planner_codec.table_provider_encode_calls() > 0
586+
assert handle_codec.table_provider_encode_calls() == 0
587+
588+
589+
def test_an_unchanged_inlining_setting_leaves_the_planner_alone():
590+
"""A no-op toggle must not rebind the session's planner.
591+
592+
``with_python_udf_inlining`` rebuilds the handle's codecs and rebinds the
593+
session's planner to them. Asking for the setting a context already has
594+
changes nothing, so it must not pay that side effect.
595+
596+
Observable only once the planner is holding some *other* handle's codec:
597+
without the guard, a defensive no-op toggle on `ctx` drags the planner back
598+
onto `ctx`'s codec and silently undoes the install below. The rebuilt
599+
codecs otherwise wrap the same inner codec, so nothing else distinguishes
600+
the two paths.
601+
"""
602+
ctx, handle_codec, _physical_codec = codec_context()
603+
ctx.set_query_planner(MyQueryPlanner())
604+
605+
planner_codec = MyLogicalExtensionCodec()
606+
ctx.with_logical_extension_codec(planner_codec) # discarded; planner keeps it
607+
gc.collect()
608+
609+
# The default is on, so this asks for what `ctx` already has.
610+
ctx.with_python_udf_inlining(enabled=True)
611+
gc.collect()
612+
613+
ctx.sql('SELECT "A" FROM numbers ORDER BY "A"').collect()
614+
assert planner_codec.table_provider_encode_calls() > 0
615+
assert handle_codec.table_provider_encode_calls() == 0
616+
617+
538618
def test_reinstalling_a_planner_rebinds_the_session_to_that_handles_codecs():
539619
"""A planner is built against the codecs of the handle it is installed from.
540620

0 commit comments

Comments
 (0)