apache / apache/datafusion-python

enable_url_table returns a handle on a different session, unlike every other with_* derivation

Đang mở
#1,708 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
api change bug
Ngôn ngữ chính
Python
Star
604
Fork
174
Merge trung bình
1 ngày 7 giờ
Pull request đã merge (30 ngày)
4

Mô tả

**Describe the bug**

`SessionContext.enable_url_table` is the only derivation in the binding that returns a handle on a *different* session than the one it was called on, and it is the only one that leaves two live handles reporting the same `session_id` with divergent state. Every other `with_*` method returns a handle wrapping the same underlying session.

The root cause is one line. Upstream's `SessionContext::enable_url_table` consumes `self` (`datafusion/core/src/execution/context/mod.rs:414`):

```rust
pub fn enable_url_table(self) -> Self {
let current_catalog_list = Arc::clone(self.state.read().catalog_list());
let factory = Arc::new(DynamicListTableFactory::new(SessionStore::new()));
let catalog_list = Arc::new(DynamicFileCatalog::new(
current_catalog_list,
Arc::clone(&factory) as Arc,
));
let session_id = self.session_id.clone();
let ctx: SessionContext = self
.into_state_builder()
.with_session_id(session_id)
.with_catalog_list(catalog_list)
.build()
.into();
factory.session_store().with_state(ctx.state_weak_ref());
ctx
}
```

Taking `self` by value means the old and new contexts are never meant to coexist, which is exactly why carrying `session_id` over is correct there. The binding at `crates/core/src/context.rs:425` takes `&self` and clones instead:

```rust
ctx: Arc::new(self.ctx.as_ref().clone().enable_url_table()),
```

That manufactures the coexistence upstream's signature prevents, and everything else is downstream of it: the must-not-outlive caveat at `crates/core/src/context.rs:428`, the "one exception" paragraph at `docs/source/contributor-guide/ffi.md:475-477`, and the state divergence below.

**To Reproduce**

```python
from datafusion import SessionContext

a = SessionContext()
b = a.enable_url_table()
print("same session_id:", a.session_id() == b.session_id())

a.sql("SET datafusion.execution.batch_size = 111").collect()
print("a", a.sql("SHOW datafusion.execution.batch_size").collect()[0].column(1).to_pylist())
print("b", b.sql("SHOW datafusion.execution.batch_size").collect()[0].column(1).to_pylist())
```

```
same session_id: True
a ['111']
b ['8192']
```

Two live sessions with independent `SessionState`, both reporting one id.

The second half of the problem is not reproducible without a built FFI extension, but it is what the existing caveat comments are about: any FFI codec, query planner, or task-context provider handed out by the receiver holds an `FFI_TaskContextProvider` bound weakly to the receiver's `Arc`. The returned context is a different allocation, so `ctx = ctx.enable_url_table()` drops the last strong reference and those components fail at query time with `TaskContextProvider went out of scope over FFI boundary`. `examples/create-context.py:41` is written as `ctx = ctx.enable_url_table()` — exactly the pattern the caveat forbids. Harmless in that example because it installs no FFI components, but it is the shape users copy.

**Expected behavior**

`enable_url_table` should behave like `with_logical_extension_codec`, `with_physical_extension_codec`, and `with_python_udf_inlining`: return a handle on the *same* underlying session, so `session_id` stays unique to a session, state cannot diverge, and FFI components bound to any handle in the lineage stay valid.

The pattern already exists in the same file. `set_session_query_planner` (`crates/core/src/context.rs:1729-1740`) mutates through `state_ref()` rather than deriving a new context, including the load-bearing `with_session_id` carry-over:

```rust
let state_ref = self.ctx.state_ref();
let factory = Arc::new(DynamicListTableFactory::new(SessionStore::new()));
let catalog_list = Arc::new(DynamicFileCatalog::new(
Arc::clone(state_ref.read().catalog_list()),
Arc::clone(&factory) as Arc,
));
{
let mut guard = state_ref.write();
*guard = SessionStateBuilder::new_from_existing(guard.clone())
.with_session_id(guard.session_id().to_string())
.with_catalog_list(catalog_list)
.build();
}
factory.session_store().with_state(Arc::downgrade(&state_ref));
```

The returned handle then shares `Arc::clone(&self.ctx)` like every sibling.

Two details for whoever picks this up:

- *Needs an idempotence guard.* `DynamicFileCatalog::new` wraps whatever catalog list is current, so calling `enable_url_table` twice nests wrappers. Upstream has the same wart, but mutating in place makes it easier to hit. `with_python_udf_inlining` (`crates/core/src/context.rs:1571-1588`) already establishes the pattern of skipping the state rebuild when the call would change nothing.
- *This is an `api change`.* `ctx = ctx.enable_url_table()` keeps working, but the original handle also gains url tables afterwards. That is the same trade `with_logical_extension_codec` already documents — it takes effect on the shared session even if the returned context is discarded — so this is bringing `enable_url_table` in line rather than introducing a new surprise. Needs a section in `docs/source/user-guide/upgrade-guides.md`, plus updates to the docstring at `python/datafusion/context.py:589`, the "one exception" paragraph at `docs/source/contributor-guide/ffi.md:475-477`, and the shared-derivation list at `docs/source/contributor-guide/ffi.md:481-486`.

**Additional context**

Found while reviewing #1679. That PR's `_derive_for_extensions` currently forks state the same way, so the "one method that mints a second `Arc`" comment at `crates/core/src/context.rs:428` is temporarily inaccurate. If #1679 lands with that fork removed, `enable_url_table` is once again the only site and that comment becomes true again.

The two are independent in both directions and should not be chained: #1679 does not need this fixed, and this does not need #1679.

Hướng dẫn đóng góp

Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này

Hướng nghiên cứu

Start at crates/core/src/context.rs:425-428 and compare enable_url_table with set_session_query_planner (1729-1740) and with_python_udf_inlining (1571-1588). Run the issue's Python reproduction and inspect examples/create-context.py:41 plus the listed FFI and upgrade-guide sections. Done means repeated calls are safe, handles share one session and state, FFI-bound components remain valid, and the referenced docs and docstring describe the shared behavior.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
python, rust
Lĩnh vực
api, documentation
Loại issue
Lỗi
Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức độ hoạt động
Sôi nổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
55/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.