Skip to content

[WIP] feat(rust): Let the returned RecordBatchReader outlive & parameters #2693

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

felipecrv
Copy link
Contributor

This is a draft PR showing a problem with the annotations of functions that return impl RecordBatchReader and take & paramemeters.

The first commit shows how I work around it. When I try to make a Box'd RecordBatchReader, the compiler complains because there is a possibility that the impl RecordBatchReader returned might be holding any of & passed in the parameters:

Screenshot 2025-04-09 at 17 50 28

When I'm forced to be explicit about lifetimes, I end up with:

struct BoxedReaderConnection(DummyConnection);

impl BoxedReaderConnection {
    pub fn get_objects<'a>(
        &'a self,
        depth: ObjectDepth,
        catalog: Option<&'a str>,
        db_schema: Option<&'a str>,
        table_name: Option<&'a str>,
        table_type: Option<Vec<&'a str>>,
        column_name: Option<&'a str>,
    ) -> Result<Box<dyn RecordBatchReader + Send + 'a>> {
        let reader = self.0.get_objects(
            depth,
            catalog,
            db_schema,
            table_name,
            table_type,
            column_name,
        )?;
        let boxed_reader: Box<dyn RecordBatchReader + Send + 'a> = Box::new(reader);
        Ok(boxed_reader)
    }
}

This is not ideal because I don't want to tie the lifetime of table_name: Option<&'a str> to the lifetime of the returned RecordBatchReader.

So I believe the proper solution is to change the adbc_core traits to explicitly declare that the RecordBatchReader can outlive the input references. Like this:

    fn get_objects<'a>(
        &'_ self,
        depth: options::ObjectDepth,
        catalog: Option<&'_ str>,
        db_schema: Option<&'_ str>,
        table_name: Option<&'_ str>,
        table_type: Option<Vec<&'_ str>>,
        column_name: Option<&'_ str>,
    ) -> Result<impl RecordBatchReader + Send + 'a>;

allowing a wrapper that boxes the RecordBatchReader to be written as

impl BoxedReaderConnection {
    pub fn get_objects<'a>(
        &'_ self,
        depth: ObjectDepth,
        catalog: Option<&'_ str>,
        db_schema: Option<&'_ str>,
        table_name: Option<&'_ str>,
        table_type: Option<Vec<&'_ str>>,
        column_name: Option<&'_ str>,
    ) -> Result<Box<dyn RecordBatchReader + Send>> {
        let reader = self.0.get_objects(
            depth,
            catalog,
            db_schema,
            table_name,
            table_type,
            column_name,
        )?;
        let boxed_reader = Box::new(reader);
        Ok(boxed_reader)
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant