feat: allow multiple Python threads to work with a single DeltaTable instance #3101
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
binding/python
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3101 +/- ##
==========================================
- Coverage 72.52% 72.38% -0.15%
==========================================
Files 128 128
Lines 41201 41296 +95
Branches 41201 41296 +95
==========================================
+ Hits 29882 29892 +10
- Misses 9408 9499 +91
+ Partials 1911 1905 -6 ☔ View full report in Codecov by Sentry. |
rtyler
force-pushed
the
feature/multithreaded-writer-2958
branch
from
3acb36f to
3804d51
Compare
rtyler
requested review from
wjones127,
fvaleye,
roeap and
ion-elgreco
as code owners
roeap
reviewed
Jan 4, 2025
python/src/lib.rs
Outdated
| Err(e) => Err(e), | ||
| } | ||
| .expect("SHOOT!"); | ||
| // TODO: XXX |
There was a problem hiding this comment.
d'oh! good catch, I did miss some of this junk in my diff review before poooshing
python/src/lib.rs
Outdated
| .map_err(PythonError::from) | ||
| .map_err(PyErr::from) | ||
| })?; | ||
| //.map_err(|_| DeltaProtocolError::new_err("table does not yet have a schema"))? |
…instance This change introduces an internal Mutex inside of RawDeltaTable which allows the PyO3 bindings to share the Python object between threads at the Python layer. PyO3 will raise a `RuntimeError: Already borrowed` for any function call which takes a mutable reference to `self`. Introducing the internal Mutex ensures that all function signatures can operate with just self-references safely. The Rust-level Mutex is a simple passthrough for most operations which do not need to modify the underlying state. The critical sections which typically need to acquire and mutate with a lock are after I/O bound operations are completed as far as I can tell, so I don't anticipate deadlock or performance issues. There is still some cleanup of errors that needs to happen to make the code here more ergonomic when blending DeltaError with PoisonError from the lock, as such right now there's a lot of ugly error mapping. Fixes delta-io#2958 Signed-off-by: R. Tyler Croy <[email protected]> Sponsored-by: Neuralink Corp.
rtyler
force-pushed
the
feature/multithreaded-writer-2958
branch
from
3804d51 to
9848ffb
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change introduces an internal Mutex inside of RawDeltaTable which allows the PyO3 bindings to share the Python object between threads at the Python layer.
PyO3 will raise a
RuntimeError: Already borrowedfor any function call which takes a mutable reference toself. Introducing the internal Mutex ensures that all function signatures can operate with just self-references safely.The Rust-level Mutex is a simple passthrough for most operations which do not need to modify the underlying state. The critical sections which typically need to acquire and mutate with a lock are after I/O bound operations are completed as far as I can tell, so I don't anticipate deadlock or performance issues.
There is still some cleanup of errors that needs to happen to make the code here more ergonomic when blending DeltaError with PoisonError from the lock, as such right now there's a lot of ugly error mapping.
Fixes #2958