jiayuasu commented on code in PR #1187:
URL: https://github.com/apache/sedona-db/pull/1187#discussion_r3911547614
##########
rust/sedona-datasource/src/format.rs:
##########
@@ -216,6 +226,10 @@ impl FileFormat for ExternalFileFormat {
#[derive(Debug, Clone)]
struct ExternalFileSource {
spec: Arc<dyn ExternalFormatSpec>,
+ /// Shared by all openers cloned from this physical scan. Keeping the lock
+ /// here scopes serialization to one scan instead of coupling independent
+ /// user streams through a process-wide lock.
+ reader_lock: Option<Arc<Mutex<()>>>,
Review Comment:
Thanks, you were right that the previous coexistence test was not enough.
The committed Python test uses two threads, two connections, and two distinct
datasets. It rendezvouses immediately before every real pyogrio backed
read_next_batch call, requires multiple successful pulls from both readers, and
validates every row for FlatGeoBuf and GeoPackage. It runs in a spawned child
process with a hard timeout and exit code reporting.
I initially asserted overlap using timestamps around each pull. A macOS
arm64 wheel run showed why that is not a sound oracle: after the barrier
releases, one very fast pull can finish before the scheduler runs the other
thread. Both scans and all rows were correct. I removed only that timing
assertion. The per batch rendezvous and real reads remain.
I also added a progress test. Reader A consumes one batch and remains alive.
Reader B completes, then reader A resumes. A temporary process wide lifetime
lock mutation makes this test time out, so holding one global mutex for the
full lifetime of a lazy reader would deadlock valid usage. That is why this PR
keeps lifecycle serialization scoped to one scan.
I also stress tested the original FlatGeoBuf path locally with open versus
read and close versus read overlap. I left that timing harness out of the PR
because it added substantial test complexity and its evidence is specific to
the tested build.
This evidence is limited to the tested drivers and builds. It is not a claim
that every GDAL driver is universally safe. If the wheel matrix reproduces a
crash under coordinated independent reads, the next design should serialize
individual native operations across the process rather than hold a lock for the
reader lifetime.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
