felipecrv commented on issue #2694: URL: https://github.com/apache/arrow-adbc/issues/2694#issuecomment-2811700735
> OK, sounds good to me. Though it sounds like Felipe's only concern was outliving the `str` parameters and not necessarily everything else, now that I look over everything again Well, moving away from `impl` as return type would be great. That's what I'm trying to do and the current annotations make it difficult due to all reference parameters being seen by the compiler as potentially copied into the returned `impl RecordBatch` (see the screenshot with all the `rust-analyzer` errors). Using `Box<RecordBatch + ...>` like most Arrow code does will make things much simpler for me and everyone else. >> we let impls specify this (using generic associated types for returned readers). > > Wouldn't that make it difficult to treat different ADBC drivers generically? I agree with @lidavidm. This makes treating all drivers as the same thing, harder. This whole investigation from my side started because I need dyn-compatible traits for `Connection` and the other `adbc_core` traits. `RecordBatchReader` is simple and dyn-compatible, let's not add non dyn-compatible stuff around it please. ---- Regarding the lifecycle of record batch readers. You've been talking about the relationship between `Connection` and `RecordBatchReader` but the more important one is not with `Connection`, but with `Statement` due to `Statement::execute()`. I feel like many users of `Statement` might let the statement die after returning a boxed `RecordBatchReader` and as a driver implementor I'm totally fine letting that happen. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
