test(core-py): document and pin the physical-table visibility contract

[ttw225]

Why

#2495 made apply_wren_on_ctx snapshot top-level catalog membership per
derivation. register_parquet/register_csv write into the pre-existing
default catalog (datafusion.public), whose internals are live-shared
with derived contexts — so tables registered after a session context was
created are still visible through query/dry_run/list_tables. The
DataFusionConnector consumer relies on exactly this, but until now the
behavior was implicit: no documentation stated it and no test guarded it.
#2504 asks for both.

What

• Document the visibility contract on register_parquet/register_csv
  (where tables land, why late registration is visible, and that brand-new
  top-level catalogs must exist before MDL construction, load_mdl, or a
  transform), plus load_mdl's concurrency contract (its exclusive borrow
  rejects overlapping calls with RuntimeError).
• Add a README section for registering local files, including the
  tableReference ↔ registered-name mapping requirement, and fix the Quick
  Start sample (it passed a manifest object and a non-existent
  remote_functions kwarg to the constructor).
• Add tests/test_physical_tables.py: register-before-load_mdl,
  register-after-derivation-without-reload (Python twin of the wren-core
  catalog_race visibility test), and register-then-reload. pyarrow
  joins the dev group to write Parquet fixtures and decode the Arrow IPC
  bytes query() returns, enabling row-level assertions.

No production code changes; no release bump expected.

Note: fixtures declare/store c_custkey as int32 on purpose — writing the
IPC stream with the logical plan's schema corrupts results when MDL types
differ from physical file types. That serialization issue was found by
these tests and is fixed separately in a follow-up fix(core-py) PR.

Test Plan

• just test-py: 38 passed (35 existing + 3 new), on this branch without
  any production change.
• just test-rs: 37 passed.
• ruff check / ruff format --check: clean on the new test file.

Part of #2504.

Summary by CodeRabbit

• New Features

  • Registered Parquet and CSV tables are now consistently discoverable in table listings and usable by queries, previews, and semantic model resolution, including across derived contexts.
  • Late registrations behave predictably for existing sessions without requiring manual reloads.

• Documentation

  • Updated quick-start/example code to use the current session construction approach.
  • Added clearer guidance on registration/load ordering, visibility expectations, and concurrency limitations for model loading.

• Tests

  • Added pinned coverage for physical-table registration visibility and semantic-resolution scenarios.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: d65b7f6e-8551-4260-ac4e-4580061dfad1

📥 Commits

Reviewing files that changed from the base of the PR and between 6e0960d and e93866d.

📒 Files selected for processing (1)

• core/wren-core-py/tests/test_physical_tables.py

🚧 Files skipped from review as they are similar to previous changes (1)

• core/wren-core-py/tests/test_physical_tables.py

---

Walkthrough

SessionContext documentation and examples now define physical table registration and MDL loading behavior. New PyArrow-based tests verify catalog visibility, direct querying, dry runs, and semantic resolution for Parquet and CSV tables.

Changes

Physical table visibility

Layer / File(s)	Summary
Registration and MDL contract core/wren-core-py/README.md, core/wren-core-py/src/context.rs	Examples and API documentation describe default catalog placement, registration timing, MDL loading order, context visibility, and concurrent-load restrictions.
Physical table test setup core/wren-core-py/pyproject.toml, core/wren-core-py/tests/test_physical_tables.py	Adds the PyArrow development dependency and shared Arrow, manifest, IPC, and Parquet fixtures.
Visibility and semantic resolution tests core/wren-core-py/tests/test_physical_tables.py	Tests registration before and after context creation, direct query visibility, and semantic resolution after load_mdl.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

• Follow-up (wren-core-py): document + test physical-table visibility through exec_ctx after per-call catalog snapshot #2504 — Covers the documentation and integration-test objectives for physical-table visibility through derived execution contexts.

Possibly related PRs

• Canner/WrenAI#2495 — Addresses related DataFusion catalog visibility and snapshot semantics across derived contexts and transforms.

Poem

A rabbit hops through tables bright,
Parquet and CSV come into sight.
MDL loads, models align,
Queries bloom in rows divine.
“Binky!” says the hare—“The contract’s right!”

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately summarizes the docs-and-tests change to the physical-table visibility contract.
Description check	✅ Passed	The description covers the motivation, changes, and test plan, with only the duplicate-check section missing.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[goldmedal]

Reviewed by checking out the branch, building the extension and re-running the suite locally: uv sync --locked --no-install-project → maturin develop → pytest gives 38 passed, and ruff check / ruff format --check are clean on the new file. I also spot-checked the doc claims against the source — clone_catalog_list (core/wren-core/core/src/mdl/context.rs) and the Rust twin test both exist and the Layer-1/Layer-2 wording matches; create_wren_ctx does leave the base context on the default datafusion.public (the MDL catalog/schema override only lands on the derived contexts inside apply_wren_on_ctx); and the old Quick Start really was wrong (remote_functions= is not a constructor kwarg). So the substance holds up.

Three things on the tests themselves:

1. The list_tables() assertion in test_register_before_load_mdl_queryable_via_semantic_layer doesn't assert anything

After load_mdl, list_tables() returns:

['customer', 'customer']

list_tables() flattens table names across every catalog, so one entry is the physical table in datafusion.public and the other is the MDL model in my_catalog.my_schema. assert "customer" in ctx.list_tables() therefore passes on the MDL model alone — it would still pass if the physical registration had gone missing, which is precisely the thing the test exists to pin.

Either drop it (the query assertion below it is doing the real work) or make it unambiguous, e.g. query datafusion.public.customer directly.

2. assert ctx.transform_sql(...) is not None is a "did not raise" check

transform_sql either returns a str or raises, so is not None can never fail on a successful call. For the scenario the test is named after — a late-registered table backing a model loaded afterwards — the interesting part is what the model resolves to. The actual output is:

SELECT customer.c_custkey FROM (SELECT customer.c_custkey FROM (SELECT __source.c_custkey AS c_custkey FROM datafusion."public".customer AS __source) AS customer) AS customer

Asserting 'datafusion."public".customer' in planned would actually pin the resolution.

3. The cross-reference to the DataFusionConnector consumer isn't accurate

The docstring in test 1 says:

The DataFusionConnector consumer never calls load_mdl; the register-then-query path it relies on is pinned by test_register_after_exec_ctx_visible_without_reload.

But test_register_after_exec_ctx_visible_without_reload deliberately constructs with an MDL — as its own comment explains, that's what forces real derived contexts. The connector constructs a bare SessionContext(), where base_ctx / ctx / exec_ctx are all the same context, so what test 2 pins is an analogous but structurally different (and stronger) scenario, not the shape the consumer actually runs.

Testing the derived-context case is the right call — it's the one that can break. But given the PR's motivation is that the connector "relies on exactly this", it seems worth either rewording the cross-reference or adding a short bare-context test alongside it (I confirmed that path works: bare SessionContext() → register_parquet → query returns the rows).

[ttw225]

Thanks for the thorough review—these are good catches. I agree the tests should pin the exact behavior rather than merely confirm that the calls don’t raise. I’ve updated them to remove the ambiguous list_tables() assertion, verify the resolved physical table in transform_sql(), and clarify the bare-context versus derived-context distinction.

[goldmedal]

Thanks @ttw225 LGTM 👍