[refactor](sql) Move runner policy out of DuckDB binding - #823
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
kaka11chen
changed the title
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
## Summary `iceberg_snapshots()` and `iceberg_metadata()` failed with `Copy not supported for TableFunctionData` through Vane's Ray runner. Both functions now copy and serialize portable bind data and use a singleton distributed scan, preventing duplicated metadata rows across workers. Binding resolves the immutable metadata file; `iceberg_metadata()` also preserves the selected snapshot and schema. Readers and cursors live in execution state. Later catalog commits or version-hint changes do not change an already-bound query. Workers need filesystem access to the bound metadata file and manifests. Regular CI and release preparation pin Vane `3c9ed18e29c586e9d5448c74440e8ea55469a749` (`0.2.0.dev657`), including AstroVela/vane#823. Ray connections can inspect client metadata using parameterized SQL; external Iceberg data still executes on Ray. The shared CI tools stay at approved commit `618aec05ad68c8c130c69505b1fd99311fc9f47d`; AstroVela/vane-extension-ci-tools#11 is not required. The production release gate continues to reject development runtimes. The existing Ray suite uses Ray for data preparation, mutations and conflict injection. The dedicated metadata suite covers complete results, filters/joins/aggregates, historical selectors, gzip, moved paths, bind-time version hints, 4,101 snapshots, and v2/v3 empty tables, concurrent appends and schema evolution. Both run in dynamic-wheel and indexed-provider CI lanes. CI fixtures use public Quay MinIO/mc release digests, including the Lakekeeper patch. ## Related issue Closes #26. #25 will contain incremental public SQL tests on top of this PR. ## Documentation impact - [ ] No user-facing documentation update is required. - [x] Documentation is updated in this PR or in a linked website PR. - [ ] <!-- docs-follow-up --> A follow-up issue is required in `AstroVela/vane-website`. README describes bind-time selection and worker filesystem requirements. The release guide records the qualified runtime and production publication prerequisites. ## Validation - Two consecutive clean local reviews of the complete PR, followed by formatting and relevant tests. - `make format-check`: passed with Black 24.10.0 and clang-format 11.0.1. - Provider/release checks: 8 + 24 tests passed. - Latest-runtime qualification: built Vane 3c9ed18e29, Avro 7f423d6970 and this PR's unchanged native Iceberg code; installed non-editable wheels in an isolated environment. The general Ray suite passed all 14 scenarios (677 s), and the metadata suite passed all 6 scenarios / 36 Ray queries (146 s). - The final parameterized provider-loading helper passed against those installed wheels. `duckdb_extensions()` and `pragma_version()` completed on a Ray connection without initializing Ray; engine identity was `v1.5.5-vane.3c9ed18e29` / `bfb327b333`. - Linux x86_64, Python 3.12.3, Ray 2.58.0, PyArrow 25.0.1; real Iceberg REST 1.10.1 and MinIO; zero-CPU Ray head and two one-CPU workers. Only the public CI signing profile was used. - Pinned image manifests and configuration blobs resolve anonymously and match their digests. Local Docker access is unavailable; container startup is validated by GitHub CI. ## Checklist - [x] The change is focused and includes tests or a reason tests are unnecessary. - [x] Public behavior and compatibility impact are documented. - [x] New dependencies, copied code, model assets, and datasets have compatible licenses and are recorded where required; no new dependencies are introduced. - [x] No credentials, private endpoints, personal paths, generated data, model weights, or build artifacts are included. Tests retain the public local-fixture credentials. - [x] Security implications of serialization, filesystem access and untrusted input have been considered. Bind payloads contain paths/selectors without catalog handles or credential objects. - [x] Native changes were compiled; Python, documentation and workflow changes passed relevant checks.
Summary
Ray queries such as
SELECT * FROM duckdb_extensions() WHERE extension_name = ?now execute on their owning client when the successful bound plan contains only declared client metadata and pure computation. Filters, metadata joins, aggregates, CTEs, views, subqueries and Relation composition use the same rule. Mixing live metadata with data sources, runner writes or explicit transports is rejected. Engine-identity queries throughpragma_version()use the same client-read declaration, so linked-extension wheel verification can retain the caller's Ray configuration.Keep this policy in Vane's existing bound-plan entry. Direct commands use native preprocessing of the original parsed AST under the connection execution lock. Named arguments and native error formatting are preserved; execute() streams the final PRAGMA result, while sql() materializes command rows with the GIL released before returning a relation. Store the fixed connection runner in DuckDB's native
ClientContextStateregistry and capture API parameters as typed AST constants before constructing nativeQueryRelation. Restore 20 files exactly to official DuckDB v1.5.5 and remove Binder routing state, the SQL-shape classifier, transient column-name capture and serialized command/source provenance. Keep the generic bound-plan handoff, earlier Relation binding adapters, #810 query cleanup and #817 write-pipeline fix.Capturing typed parameters also makes large media values participate in parsed-expression hashing. Hash large LIST/ARRAY children in bounded batches, retaining the existing hash order and NULL semantics, so 4K image binding and image-to-tensor conversion stay within the existing memory budgets. The native resize regression checks execution with the Python implementation forbidden; EXPLAIN can preserve the original SQL column name. Isolate generic Float32 and fixed UInt8 image memory tests in separate subprocesses. Removing bind-time hash scratch exposed their dependency on retained allocator space: generic execution now has a 640 MiB growth budget, while the fixed-layout 192/224 MiB limits remain unchanged.
Native binding can read schemas, autoload extensions and evaluate arguments before admission. JSON table-function binding can report native nested-SQL errors at this stage; successfully bound client-only functions are rejected before runner execution. Folded values such as
getvariable()and completed DESCRIBE rows can become portable data. Extension declarations and surviving bound expressions define routing; arbitrary dependencies hidden in bind data, optimization or physical planning are outside this contract. No compatibility layer or execution fallback is added.Related issue
close: #822
Documentation impact
AstroVela/vane-website.README documents metadata composition, native binding effects, explicit transaction timing, completed command rows, extension requirements and implicit expanded column names after parameter capture.
Validation
The final test-contract update changes only README and two test files. Two consecutive clean local static reviews passed before validation. It uses the exact base wheel and signed
loadable_extension_demoprovider from CI run 35003377561, with the complete CPU build/test dependencies installed in a fresh environment (Python 3.12.3, official DuckDB 1.5.5, CPU-only PyTorch).patchelftool) and 6,025 in shard 2. The second shard had the tool available before its affected fixtures ran.range(nextval(...))JSON input produces the same native bindingInternalExceptionin official DuckDB 1.5.5 and Vane local-fast. The revised tests check execution-time side effects and native binding-error propagation separately, consistent with the documented admission contract; no production or DuckDB source change is needed.Two consecutive clean local static reviews preceded the final native compilation and test batch. Non-editable Release build passed on Linux x86-64 / Python 3.12.3. Affected tests: 2,082 non-Ray passed (one existing public-API skip), 14 real-Ray passed, and 4 TPCH command tests passed. 1,257 base release tests passed (1,226 non-Ray + 31 real-Ray).
Native hashing validation passed 859 assertions in four test cases: the new
[hash]regression plus DuckDB’s existinghash_func.test,hash_constant.testandarray_hash.test. The seven original large-image memory regressions passed with their memory limits unchanged; the native resize regression passed with the Python implementation forbidden.Media validation covers 1,835 unchanged non-Ray cases and all eight isolated Python/native × generic/fixed memory variants. The unchanged production sources also passed the native/media CI gates on all five supported Python versions (3.10–3.14). The same final native binaries passed 45 shared-cluster Ray media tests and two two-node runtime replacement tests with the signed combined native_media provider installed. The follow-up changes are limited to documentation and tests, so the tested native SourceID is unchanged.
VANE_TEST_NATIVE_MEDIA_EXTENSION="$PWD/build/python-release/vane_extensions/native_media.duckdb_extension" \ scripts/run_installed_pytest.sh \ tests/fast/test_image_operators.py::test_hd_constant_image_does_not_expand_input_batch \ tests/fast/test_image_transforms.py::test_transforms_keep_large_constant_input_and_output_once \ -q --tb=shortThe TPCH fixture verifies native direct execution and the independent rejection of unregistered dynamic extensions during connection snapshot export. Portable completed-command rows are covered separately without an unregistered extension. No TPCH artifact is published.
The unmodified
verify_vane_wheelfrom vane-extension-ci-tools also passed with the built Vane wheel and its statically linked Parquet extension in two fresh virtual environments: once with no caller runner setting and once withVANE_RUNNER=ray. Both checked static linkage, LOAD, the fork version and SourceID and preserved the caller environment. This did not use the local-fast override proposed in AstroVela/vane-extension-ci-tools#11.Checklist
licenses and are recorded where required.
weights, or build artifacts are included.
and untrusted input have been considered.
changes passed relevant checks.