You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mbrobbel (via GitHub)" <gi...@apache.org> on 2025/04/17 09:27:32 UTC
[PR] feat(rust)!: use GATs for return types in `Connection` and `Statement` traits [arrow-adbc]
mbrobbel opened a new pull request, #2713:
URL: https://github.com/apache/arrow-adbc/pull/2713
Following the discussion in #2694 - this changes the `impl Trait` return types to use generic associated types. This is a breaking change.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust)!: use GATs for return types in `Connection` and `Statement` traits [arrow-adbc]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel closed pull request #2713: feat(rust)!: use GATs for return types in `Connection` and `Statement` traits
URL: https://github.com/apache/arrow-adbc/pull/2713
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust)!: use GATs for return types in `Connection` and `Statement` traits [arrow-adbc]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #2713:
URL: https://github.com/apache/arrow-adbc/pull/2713#issuecomment-2816172396
> > If you don't want to trust the ADBC driver implementations to handle the reference counting and weak references to connection (I would) you can keep an inner Arc inside every Connection and avoid trying to track this complex relationship statically.
>
> Is what you are describing provided by the driver manager?
>
> https://github.com/apache/arrow-adbc/blob/7f1dfca741c298209e213d00d785c7a220bdc0bb/rust/core/src/driver_manager.rs#L38-L41
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust)!: use GATs for return types in `Connection` and `Statement` traits [arrow-adbc]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #2713:
URL: https://github.com/apache/arrow-adbc/pull/2713#issuecomment-2813193986
This definitely seems correct in the sense that it is statically communicating the constraints of the ADBC object lifetimes as written in the spec correctly. It is also true that static lifetime annotations make writing wrappers (like the one Felipe demoed) highly unenjoyable and/or impossible to combine with calls to another library that is highly enthusiastic about lifetime annotations. I would definitely prefer to return `Box<dyn RecordBatchReader>`...as Felipe pointed out, this type of annotation can always be added at a higher level of abstraction.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust)!: use GATs for return types in `Connection` and `Statement` traits [arrow-adbc]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #2713:
URL: https://github.com/apache/arrow-adbc/pull/2713#issuecomment-2816186776
> > If you don't want to trust the ADBC driver implementations to handle the reference counting and weak references to connection (I would) you can keep an inner Arc inside every Connection and avoid trying to track this complex relationship statically.
>
> Is what you are describing provided by the driver manager?
>
> https://github.com/apache/arrow-adbc/blob/7f1dfca741c298209e213d00d785c7a220bdc0bb/rust/core/src/driver_manager.rs#L38-L41
What I'm suggesting would require a `ManagedRecordBatchReader` that wraps a weak-pointer to the managed connection and the underlying record batch reader that comes from the FFI.
So `ManagedRecordBatchReader::Next()` tries to upgrade the weak reference to a strong one, then it calls next on the underlying reader. If the upgrade fails, `Next()` immediately returns an error.
This would make readers (1) safe when the underlying reader in the driver doesn't take care of that, and (2) dangling readers wouldn't prevent connection destruction since the readers hold a weak reference to the connection. Another good consequence of (2) is that connection destruction is not blocked by all the record batch readers that originated from that connection.
An alternative to that is trusting that the driver itself takes care of that (they should) and inheriting the safety (or lack of) properties of the driver when it comes to managing the relationship between readers and connections.
a) track relationship with static types (too restrictive and high cognitive overload)
b) tracking relationship dynamically with weak references (safe, flexible, easy to use, but adds another layer of ref-counting)
c) trusting that the driver's `RecordBatchReader::Next()` will fail when the shared connection resources are not available anymore (as safe as the driver is, easy to use, flexible, no extra layer of ref-counting)
Link to docs on `Weak` refs: https://doc.rust-lang.org/std/sync/struct.Weak.html#method.upgrade (I believe what we have here is a textbook application of weak references. It's to deal with these problems that they exist).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust)!: use GATs for return types in `Connection` and `Statement` traits [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #2713:
URL: https://github.com/apache/arrow-adbc/pull/2713#issuecomment-2817442161
`Box<RecordBatchReader>` sounds like the pragmatic choice to me here.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(rust)!: use GATs for return types in `Connection` and `Statement` traits [arrow-adbc]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on PR #2713:
URL: https://github.com/apache/arrow-adbc/pull/2713#issuecomment-2813149682
> Is adding one dedicated type per function that returns a record batch reader sensible?
We could reduce this to one associated type for all the returned readers?
> If an ADBC driver can be used from languages like C++ and Python, that don't have statically checked lifetimes, it should be possible to use from Rust without all this complexity.
> If you don't want to trust the ADBC driver implementations to handle the reference counting and weak references to connection (I would) you can keep an inner Arc inside every Connection and avoid trying to track this complex relationship statically.
Is what you are describing provided by the driver manager?
https://github.com/apache/arrow-adbc/blob/7f1dfca741c298209e213d00d785c7a220bdc0bb/rust/core/src/driver_manager.rs#L38-L41
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org