-
Notifications
You must be signed in to change notification settings - Fork 208
feat: support postgresql #286
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
Conversation
| 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 || { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
|
@houqp Is this going to merge? I aslo have interesting about this |
|
@houqp We are very interested in this feature! When can we expect its implementation? |
|
sorry for the delay, will take a look at this later today |
|
@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 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. |
|
hopefully #291 should fix it. |
|
@holicc could you rebase to get a clean CI run? I have fixed the main branch. |
|
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: roapi/.github/workflows/build.yml Line 292 in b8b772a
|
It appears that we need to install
@houqp I don't know how to trigger the flow. Can you help me out? |
|
@holicc I was suggesting we don't use the
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. |
|
Thank you @holicc ! |
|
Recently, I found that we can simplify the code base by using the get_arrow API from connectorx. 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()],
)?)
}
} |
|
Yes, let's do that @holicc ! |
#177