Re: [PR] feat(c/sedona-extension): Add FFI table provider, exec plan, and synchronous stream exchange [sedona-db]

["james-willis (via GitHub)" <gi...@apache.org>]

james-willis commented on PR #1004:
URL: https://github.com/apache/sedona-db/pull/1004#issuecomment-4860745063

   I havent had time to review this myself yet, but here is a review from fable/ultracode. Do with it what you will knowing its just from the AI.
   
   Correctness bugs
   
     1. c/sedona-extension/src/streaming.rs:312 — The imported stream blocks a tokio worker thread in a synchronous recv() while the producer-side StreamWorker needs the same runtime via Handle::block_on, deadlocking when producer and consumer share a runtime. (Any plan that
     spawns tasks — e.g. contains RepartitionExec — exported and executed on the same runtime hangs forever once the runtime's workers are all blocked in recv(); deterministic on a current-thread runtime. The test comment at streaming.rs:352 about avoiding
     tokio::time::sleep because it "can deadlock with block_on" shows the hazard was already encountered.)
     2. Runtime-liveness regression, three sites — python/sedonadb/src/dataframe.rs:453, rust/sedona-adbc/src/statement.rs:102, and r/sedonadb/src/rust/src/dataframe.rs:118 (R site PLAUSIBLE, other two CONFIRMED). The new StreamingRecordBatchReader holds only a
     tokio::runtime::Handle where the deleted readers held Arc<Runtime>, so the runtime can shut down while an exported stream is still being read. (caps = df.execute_stream().__arrow_c_stream__(); del df; del con drops the last Arc<Runtime>; the next capsule read calls
     block_on on a shut-down runtime and the consumer gets a spurious "Worker thread terminated" error. Same for ADBC releasing the statement before draining the reader, or R garbage-collecting the context mid-stream.)
     3. python/sedonadb/src/dataframe.rs:676 — __sedonadb_table_provider__ exports the view with a bare SessionContext::new(), and the producer side uses that empty session for both scan() planning and task_ctx() execution — the comment claiming "the consuming side will use
     its own session" is wrong; the consumer's session is discarded at table_provider.rs:345. (sd.read_parquet('s3://…') imported into another context fails at execute time with "No suitable object store found" because the bare session lacks the producer's object-store
     registrations, options, and Sedona planner extensions.)
     4. c/sedona-extension/src/execution_plan.rs:238 — Every FFI error path writes *err = SedonaCError::new(...), which first runs Drop on the previous contents of *err, but the header never requires callers to zero-initialize SedonaCError. (A C consumer passing an
     uninitialized stack SedonaCError triggers a read of a garbage release function pointer and a call through it on the first error — jump to a random address. Applies to every *err assignment in execution_plan.rs and table_provider.rs.)
     5. get_schema has no error channel — c/sedona-extension/src/execution_plan.rs:171–182 and c/sedona-extension/src/table_provider.rs:141 (three angles converged here). The callback is declared void, and on FFI_ArrowSchema::try_from failure both implementations silently
     return without writing *out — unlike every sibling callback, which takes a SedonaCError and returns int. (A C consumer following the Arrow convention of passing an uninitialized ArrowSchema then calls a garbage release() — UB; the Rust importer parses an empty schema
     and surfaces a misleading "null format" error with the real cause swallowed. This is an ABI-shape issue worth fixing before the header is published.)
     6. c/sedona-extension/src/sedona_extension.h:228 — SedonaCExpr.get_property_schema is missing the struct ArrowSchema* out parameter that the equivalent callbacks on SedonaCExecutionPlan (line 283) and SedonaCTableProvider (line 341) have, so the callback cannot return a
     schema at all. (The doc on SedonaCExpr.get_property says the type "may be retrieved with the get_property_schema callback," but the signature has no output slot. Fixing the function-pointer type after the header ships breaks every compiled consumer, so it should be
     corrected now.)
     7. python/sedonadb/src/import_from.rs:49 — Standard DataFusion FFI interop is dropped: import now only recognizes __sedonadb_table_provider__; nothing handles __datafusion_table_provider__ anymore. (Objects from datafusion-python or delta-rs previously imported
     zero-copy via FFI_TableProvider; now they silently fall through to single-scan __arrow_c_stream__ materialization, or fail with "Can't create table provider" if they have no stream dunder.)
     8. c/sedona-extension/src/streaming.rs:149 — Cancellation no longer stops query execution: the deleted wait_for_future_from_rust dropped the polled future on Ctrl+C, but the new worker thread is stuck uninterruptibly in runtime.block_on(stream.next()). (Ctrl+C during a
     slow batch returns control to Python, but the worker keeps burning CPU and the stream's memory/file handles can't be dropped until the batch completes — for a never-yielding stream, they leak for the life of the process.)
     9. c/sedona-extension/src/extension.rs:188 — sedona_c_noop_release doesn't set self->release = NULL, violating the header's own contract ("Implementations of this callback must set self->release to NULL") for the UNKNOWN_SEDONA_C_ERROR fallback. (A consumer that uses
     release == NULL to detect a released error sees a still-"valid" error and may double-release or keep reading it; while (err.release) err.release(&err); spins forever.)
     10. c/sedona-extension/src/streaming.rs:261 — Iterator::next detects cancellation by downcasting the error fetch_next_batch itself constructed (ExternalError → io::Error → ErrorKind::Interrupted). (If the inner stream yields its own interrupted-io::Error — e.g. an
     interrupted object-store read — the reader misclassifies it as user cancellation, sets cancelled = true, and permanently returns None, silently truncating remaining batches. Simpler: have fetch_next_batch return an explicit Cancelled variant or set the flag itself.)
   
     Efficiency
   
     11. c/sedona-extension/src/table_provider.rs:86 — ExportedTableProvider::scan spawns a brand-new OS thread per scan call and immediately join()s it, parking the calling thread (typically a consumer tokio executor thread) for the entire producer-side planning. (For
     object-store-backed tables, planning does network I/O that can take seconds; several concurrent queries against imported tables stall the consumer runtime. Cheaper: one long-lived planning thread, or a oneshot channel awaited asynchronously.)
     12. c/sedona-extension/src/streaming.rs:139 — The zero-capacity rendezvous protocol (worker only fetches after a request arrives) fully serializes producer fetch and consumer processing, with two context switches per batch and one dedicated OS thread per partition
     stream. (Per-batch wall time is T_produce + T_consume instead of max of the two — roughly doubling a 1000-batch export through Python when the costs are comparable; a 32-partition imported plan also spawns 32 extra threads per execution. Cheaper: eager fetch into a
     bounded channel of capacity 1–2 for one-batch pipelining.)
     13. c/sedona-extension/src/utils.rs:158 — Every property fetch makes a second FFI roundtrip to get_property_schema even though the exporter always answers Utf8. (EXPLAIN ANALYZE or repeated metrics() polling doubles FFI traffic and does a CString/Field/FFI_ArrowSchema
     allocate-and-parse cycle per read. Cheaper: fetch the property schema once and cache it.)
   
     Altitude
   
     14. r/sedonadb/src/rust/src/dataframe.rs:134 — The R bindings still export/import via the version-locked datafusion-ffi FFI_TableProvider, so the new version-agnostic C mechanism is wired into Python only and two parallel table-provider FFI mechanisms must now be
     maintained. (Every future exchange change — pushdown, cancellation, error surfaces — must be made twice, and an R package and Python package built against different DataFusion versions still can't exchange providers, defeating the PR's ABI-stability goal for one of the
     bindings.)
   
     Reuse / simplification
   
     15. c/sedona-extension/src/utils.rs:234 and :311 — Four near-verbatim copies of the property-fetch protocol (get_plan_property, get_plan_string_property, get_table_provider_string_property, get_table_provider_property_data_type): identical CString setup, unsafe call,
     errno handling, and schema lookup, differing only in struct type and the final decode step. (~180 lines of duplicated unsafe FFI code; any fix to errno mapping or error freeing must land in four places, and a missed copy gives divergent behavior between plan and
     provider property reads — partially masked by the silent default-to-TableType::Base fallback.)
     16. c/sedona-extension/src/streaming.rs:243 — The cancellation error (io::Error with ErrorKind::Interrupted) is constructed at two sites with no shared constructor, and next() re-detects it by downcast — an invisible coupling. (Editing one copy to a different error type
     without updating the downcast means the cancelled flag is never set, and the FFI stream consumer re-blocks on the still-running query instead of terminating — a hang that only manifests on the Ctrl+C path. Closely related to finding 10.)
     17. c/sedona-extension/src/table_provider.rs:475 — The six test_roundtrip_* tests each repeat the same 10-line export/import/register preamble with only the SQL string differing, while a suitable helper (setup_imported_provider_with, line 731) already exists in the same
     module. (Any signature change to ExportedTableProvider::new or ImportedTableProvider::try_new requires editing six copies; a parameterized (sql, expected) helper cuts ~60 lines.)
     18. c/sedona-extension/src/execution_plan.rs:318 — Stale copy-pasted name ImportedTableProviderExec survives in the Debug impl (lines 318, 322), the try_new doc comment (line 328), and the with_new_children error message (line 483) of a type actually named
     ImportedSedonaCExec. (Someone debugging greps for a type that doesn't exist. Use self.name() or fix all four.)
     19. c/sedona-extension/Cargo.toml:44 — sedona-geometry is added as a dependency but nothing in the crate references it. (Every build of the extension crate — and Python/R/ADBC downstream — compiles and links it for nothing. Delete the line.)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@sedona.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org