Skip to content

Conversation

@holicc
Copy link
Contributor

@holicc holicc commented Jul 20, 2023

PostgresSource::new(config.into(), tls, 2)
.map_err(|e| ColumnQError::Database(e.to_string()))?;
let queries = queries.clone();
let task = tokio::task::spawn_blocking(move || {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something here, why do we need to fetch postgres data with blocking code here?

Copy link
Contributor Author

@holicc holicc Jul 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't block here, will cause err: Cannot start a runtime from within a runtime.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is specific to the PostgresSource? I will take a closer look at it this weekend, thanks for the patch!

If this is expected, then we should also file an upstream bug report, async call context should be supported as first class citizen in connectorx.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it's specific to the PostgresSource.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@holicc Do you know why have this error Cannot start a runtime from within a runtime?

Copy link
Contributor Author

@holicc holicc Aug 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuyang-ok In somewhere create a new runtime when using PgSource

@yuyang-ok
Copy link

@houqp Is this going to merge? I aslo have interesting about this

@BaJIkupuR
Copy link

@houqp We are very interested in this feature! When can we expect its implementation?

@houqp
Copy link
Member

houqp commented Aug 22, 2023

sorry for the delay, will take a look at this later today

@houqp
Copy link
Member

houqp commented Aug 24, 2023

@holicc do you need help fixing the CI? I was planning to dig into that, but don't want to step on your toes.

@holicc
Copy link
Contributor Author

holicc commented Aug 25, 2023

@holicc do you need help fixing the CI? I was planning to dig into that, but don't want to step on your toes.

@houqp Yes, I'm not sure how to fix it.

@houqp
Copy link
Member

houqp commented Aug 27, 2023

@holicc this is because the gcs setup in CI was failing, for some reasons, the curl command was not able to store the parquet file there. Looking into it more now.

@houqp
Copy link
Member

houqp commented Aug 27, 2023

hopefully #291 should fix it.

@houqp
Copy link
Member

houqp commented Aug 27, 2023

@holicc could you rebase to get a clean CI run? I have fixed the main branch.

@houqp
Copy link
Member

houqp commented Aug 28, 2023

OK, now we are back to MacOS openssl cross build issues :(

@holicc I think the quickest fix is to disable the postgres feature in the mac build job:

cargo build --bin roapi --features database --target aarch64-apple-darwin
.

@holicc
Copy link
Contributor Author

holicc commented Aug 29, 2023

OK, now we are back to MacOS openssl cross build issues :(

@holicc I think the quickest fix is to disable the postgres feature in the mac build job:

cargo build --bin roapi --features database --target aarch64-apple-darwin

.

It appears that we need to install openssl in the build environment.

@houqp I don't know how to trigger the flow. Can you help me out?

@houqp
Copy link
Member

houqp commented Aug 29, 2023

@holicc I was suggesting we don't use the --features database in the macos crossbuild job, but instead build explicitly with the following flags to exclude postgres for mac for now:

  • database-sqlite
  • database-mysql

Cross building openssl on mac is going to be a major undertake unfortunately. So we can look into fixing that later.

@holicc
Copy link
Contributor Author

holicc commented Aug 29, 2023

@holicc I was suggesting we don't use the --features database in the macos crossbuild job, but instead build explicitly with the following flags to exclude postgres for mac for now:

  • database-sqlite
  • database-mysql

Cross building openssl on mac is going to be a major undertake unfortunately. So we can look into fixing that later.

My bad. I thought it was a simple problem.

@houqp houqp merged commit ff36177 into roapi:main Aug 29, 2023
@houqp
Copy link
Member

houqp commented Aug 29, 2023

Thank you @holicc !

@holicc
Copy link
Contributor Author

holicc commented Sep 7, 2023

Recently, I found that we can simplify the code base by using the get_arrow API from connectorx.
@houqp Do you think we should modify the code like this?

impl DatabaseLoader {
        pub fn to_mem_table(
            &self,
            t: &TableSource,
        ) -> Result<datafusion::datasource::MemTable, ColumnQError> {
            debug!("loading database table data...");
            let queries = CXQuery::naked(format!("SELECT * FROM {}", t.name));
            let source = SourceConn::try_from(t.get_uri_str())
                .map_err(|e| ColumnQError::Database(e.to_string()))?;
            let destination = connectorx::get_arrow::get_arrow(&source, None, &[queries])
                .map_err(|e| ColumnQError::Database(e.to_string()))?;
            Ok(datafusion::datasource::MemTable::try_new(
                destination.arrow_schema(),
                vec![destination.arrow().unwrap()],
            )?)
        }
    }

@houqp
Copy link
Member

houqp commented Sep 7, 2023

Yes, let's do that @holicc !

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.

4 participants