Skip to content
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

Support tencent cloud COS storage in datafusion-cli #9734

Merged
merged 4 commits into from
Mar 25, 2024
Merged

Conversation

harveyyue
Copy link
Contributor

@harveyyue harveyyue commented Mar 22, 2024

Which issue does this PR close?

NA

Rationale for this change

What changes are included in this PR?

Support cos:// schema as below:

CREATE EXTERNAL TABLE test
STORED AS PARQUET
OPTIONS(
    'aws.access_key_id' 'xxx',
    'aws.secret_access_key' 'xxx',
    'aws.cos.endpoint' 'https://cos.ap-singapore.myqcloud.com'
)
LOCATION 'cos://bucket/path/file.parquet';

Are these changes tested?

yes, unit test

Are there any user-facing changes?

no

@@ -388,7 +411,7 @@ Note that the `endpoint` format of oss needs to be: `https://{bucket}.{oss-regio
CREATE EXTERNAL TABLE test
STORED AS PARQUET
OPTIONS(
'service_account_path' '/tmp/gcs.json',
'gcp.service_account_path' '/tmp/gcs.json',
Copy link
Contributor

Choose a reason for hiding this comment

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

is it related to cos onboard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, add the missing prefix gcp and aws to related external table options.

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

Thanks @harveyyue for your contribution.
Please add more details to the PR description, this will makes easier to other people what is the PR without going though the code.

Other than that it looks good, I'll leave it for other reviewers to have a second look.

@@ -415,6 +424,15 @@ pub(crate) async fn get_object_store(
let builder = get_oss_object_store_builder(url, options)?;
Arc::new(builder.build()?)
}
"cos" => {
let Some(options) = table_options.extensions.get::<AwsOptions>() else {
Copy link
Contributor

Choose a reason for hiding this comment

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

In follow up we need to refactor this part as for different schemas its too much of the common code

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, agree with you.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to file a ticket to do so 🙏

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you @harveyyue and @comphead for the review

I took the liberty of fixing the clippy CI test and merging up from main and pushed the commits to your branch

@@ -415,6 +424,15 @@ pub(crate) async fn get_object_store(
let builder = get_oss_object_store_builder(url, options)?;
Arc::new(builder.build()?)
}
"cos" => {
let Some(options) = table_options.extensions.get::<AwsOptions>() else {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to file a ticket to do so 🙏

@alamb alamb changed the title Support tencent cloud COS storage Support tencent cloud COS storage in datafusion-cli Mar 24, 2024
@alamb alamb merged commit ad89ff8 into apache:main Mar 25, 2024
24 checks passed
@alamb
Copy link
Contributor

alamb commented Mar 25, 2024

Thanks again @harveyyue and @comphead

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.

3 participants