Skip to content

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jun 6, 2025

Which issue does this PR close?

Rationale for this change

I want to be able to access public s3 buckets without providing (valid) s3 credentials

What changes are included in this PR?

  1. Add skip_signature option to datafusion-cli CREATE EXTERNAL TABLE
  2. Default to skip_signature when other credentials are not provided
  3. Update documentation

Before this PR:

DataFusion CLI v47.0.0
> CREATE EXTERNAL TABLE nyc_taxi_rides
STORED AS PARQUET LOCATION 's3://altinity-clickhouse-data/nyc_taxi_rides/data/tripdata_parquet/';
Object Store error: Generic S3 error: the credential provider was not enabled

After this PR:

DataFusion CLI v48.0.0
> CREATE EXTERNAL TABLE nyc_taxi_rides
STORED AS PARQUET LOCATION 's3://altinity-clickhouse-data/nyc_taxi_rides/data/tripdata_parquet/';
selec0 row(s) fetched.
Elapsed 1.575 seconds.

> select count(*) from nyc_taxi_rides;
+------------+
| count(*)   |
+------------+
| 1310903963 |
+------------+
1 row(s) fetched.
Elapsed 3.011 seconds.

Are these changes tested?

Yes, new unit tests are added and I tested it manually

For example, if you provide credentials, they take precidence over the signature:

AWS_ACCESS_KEY_ID=foo AWS_SECRET_ACCESS_KEY=bar  cargo run -p datafusion-cli
> CREATE EXTERNAL TABLE nyc_taxi_rides
STORED AS PARQUET LOCATION 's3://altinity-clickhouse-data/nyc_taxi_rides/data/tripdata_parquet/';
Object Store error: Generic S3 error: Error performing list request: Error performing GET https://s3.us-east-1.amazonaws.com/altinity-clickhouse-data?list-type=2&prefix=nyc_taxi_rides%2Fdata%2Ftripdata_parquet%2F in 134.200375ms - Server returned non-2xx status code: 403 Forbidden: <?xml version="1.0" encoding="UTF-8"?>
<Error><Code>InvalidAccessKeyId</Code><Message>The AWS Access Key Id you provided does not exist in our records.</Message><AWSAccessKeyId>foo</AWSAccessKeyId><RequestId>ZAEM63Q02FQXYMTA</RequestId><HostId>mYh2PUtKzDxjrPA4vQm4d+Qae9TiNpCUDDTS5BP4jTayKVE4BQbSpT/+HSIAdzt3lne6G0sxNmE=</HostId></Error>

But you can override this with SKIP_SIGNATURE

> CREATE EXTERNAL TABLE nyc_taxi_rides
STORED AS PARQUET LOCATION 's3://altinity-clickhouse-data/nyc_taxi_rides/data/tripdata_parquet/' OPTIONS(AWS.SKIP_SIGNATURE 'true');
0 row(s) fetched.
Elapsed 1.455 seconds.

Are there any user-facing changes?

Easier to use datafusion-cli

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jun 6, 2025
builder = builder.with_credentials(credentials);
} else {
debug!("No credentials found, defaulting to skip signature ");
builder = builder.with_skip_signature(true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this changes the default so we don't try and create a signature if no credentials can be extracted

"allow_http" => {
self.allow_http.set(rem, value)?;
}
"skip_signature" | "nosign" => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is support for passing through skip_signature

) LOCATION '{location}'"
);

let ctx = SessionContext::new();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at this with whitespace blind diff https://github.com/apache/datafusion/pull/16300/files?w=1

makes it easier to see what is happening

Specifically, I just refactored the parsing / table_options extraction into a separate function to reduce repetition.

Otherwise the existing tests are the same

@alamb alamb marked this pull request as ready for review June 6, 2025 12:54
@blaginin blaginin self-requested a review June 9, 2025 19:56
Copy link
Contributor

@blaginin blaginin left a comment

Choose a reason for hiding this comment

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

🙌🏻

// we can call `provide_credentials` here.
let credentials = match credentials.provide_credentials().await {
Ok(_) => Some(credentials),
Err(e) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: does it make sense to expect specific errors? For example, for CredentialsError::InvalidConfiguration or InvalidConfiguration::ProviderTimedOut, we probably still want to raise since those mean the creds could be set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a great idea -- fixed in 37d1132

@blaginin blaginin merged commit a91e042 into apache:main Jun 11, 2025
31 checks passed
@alamb alamb deleted the alamb/nosign-cli branch June 11, 2025 13:02
@alamb
Copy link
Contributor Author

alamb commented Jun 11, 2025

Thanks @blaginin !

@alamb
Copy link
Contributor Author

alamb commented Jun 11, 2025

There appears to be some problem with this code in PR:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support datafusion-cli access to public S3 buckets that do not require authentication

2 participants