Support create object store source tables without depending on environment variables #5732

r4ntix · 2023-03-25T05:22:44Z

Which issue does this PR close?

Closes #5731

Rationale for this change

See #5731

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

… store table through ListingTableFactory

yjshen · 2023-03-26T02:01:40Z

datafusion/core/Cargo.toml

@@ -85,7 +81,7 @@ lazy_static = { version = "^1.4.0" }
 log = "^0.4"
 num-traits = { version = "0.2", optional = true }
 num_cpus = "1.13.0"
-object_store = "0.5.4"
+object_store = { version = "0.5.4", features = ["aws"] }


I'm concerned about introducing cloud storage features into datafusion/core. Perhaps we could push down options into object_store through a general-purpose interface and handle the s3/oss logic there.

Thinks for this feedback.
I think the question here is whether we enable access to cloud storage by default for datafusion. If yes, it is the same whether to put the options in datafusion or in object_store. If not, I can make this as an optional feature for datafusion/core, and turn it on by default in the datafusion-cli.
Which do you think is better? happy to discuss this 😄

I think the question here is whether we enable access to cloud storage by default for datafusion. If yes, it is the same whether to put the options in datafusion or in object_store. If not, I can make this as an optional feature for datafusion/core, and turn it on by default in the datafusion-cli.

I think the desire has been to keep ObjectStore specific dependencies out of DataFusion core (though it would be good to document that somewhere). DataFusion is already a large codebase to begin with and given it has hooks to extend the functionality I think we should be leveraging the extension points.

alamb

Thank you for the PR @r4ntix -- I agree with @yjshen that we are trying to keep DataFusion agnostic to different object store implementations, so ideally this configuration would be handled by users of DataFusion (such as the datafusion-cli).

Among other reasons, we currently don't have any CI coverage for these object store implementations.

@r4ntix -- could you possibly try to move the object store configuration into datafusion-cli? Perhaps you can look at how ballista does this: https://github.com/apache/arrow-ballista

alamb · 2023-03-28T20:45:25Z

datafusion/core/Cargo.toml

@@ -85,7 +81,7 @@ lazy_static = { version = "^1.4.0" }
 log = "^0.4"
 num-traits = { version = "0.2", optional = true }
 num_cpus = "1.13.0"
-object_store = "0.5.4"
+object_store = { version = "0.5.4", features = ["aws"] }


I think the question here is whether we enable access to cloud storage by default for datafusion. If yes, it is the same whether to put the options in datafusion or in object_store. If not, I can make this as an optional feature for datafusion/core, and turn it on by default in the datafusion-cli.

I think the desire has been to keep ObjectStore specific dependencies out of DataFusion core (though it would be good to document that somewhere). DataFusion is already a large codebase to begin with and given it has hooks to extend the functionality I think we should be leveraging the extension points.

alamb · 2023-03-28T21:18:10Z

datafusion/core/src/datasource/listing_table_factory.rs

+
+                state
+                    .runtime_env()
+                    .register_object_store(table_path.as_ref(), store);


I wonder if you can move this code for registering the object store dynamically out of core and into datafusion-cli ?

Thanks for the suggestion!

Considering that there is already a dependency on object_store in the current datafusion core, I think it would be a natural thing to support different object store implementations directly in datafusion core.

@alamb @yjshen
Is it possible to make this as an optional feature of datafusion core? That way it would be optional and out-of-the-box for CLI or other services that use Datafusion.

@alamb @yjshen I created a new branch and moved the code from datafusion core to datafusion-cli: r4ntix@f988046

Because the necessary info of object store is dynamically fetched from the SQL(cmd.options), there is an additional sql parsing overhead here:

async fn exec_and_print( ctx: &mut SessionContext, print_options: &PrintOptions, sql: String, ) -> Result<()> { let now = Instant::now(); // parsing sql to get external table information let plan = ctx.state().create_logical_plan(&sql).await?; let df = match plan { LogicalPlan::CreateExternalTable(cmd) => { create_external_table(&ctx, &cmd)?; ctx.sql(&sql).await? } _ => ctx.sql(&sql).await?, }; let results = df.collect().await?; print_options.print_batches(&results, now)?; Ok(()) }

I think r4ntix@f988046 looks good too me -- if you want to get rid of the overhead of calling sql twice you could perhaps allow the dispatch code on SessionContext diectly.

Like maybe add a

impl SessionContext { pub async fn sql(&self, sql: &str) -> Result<DataFrame> { // create a query planner let plan = self.state().create_logical_plan(sql).await?; self.execute_logical_plan(plan)? } // Execute a LogicalPlan (including command such as CreateExternalTable, etc) fn execute_logical_plan(&self, plan: LogicalPlan) -> Result<DataFrame> { match plan { ... }

https://docs.rs/datafusion/latest/src/datafusion/execution/context.rs.html#318-320

Ok, I'll submit a new commit for this

…core and into datafusion-cli

r4ntix · 2023-04-01T09:48:39Z

Hi @alamb @yjshen Thank you for the suggestions! I've submitted a new commit, cloud you help review it?

alamb

Thanks @r4ntix -- I think this looks good to me. Thank you for moving the code.

I had some small documentation suggestions.

My biggest concern / question is about testing -- specifically do you think it is worth writing tests (especially for error conditions) for options? I am mostly worried about potentially breaking this functionality with some future refactor but not catching that because of lack fo test coverage

alamb · 2023-04-03T19:19:12Z

docs/source/user-guide/cli.md

+- session_token
+- region
+
+It is also simplify sql statements by environment variables.


Suggested change

It is also simplify sql statements by environment variables.

It is also possible to simplify sql statements by environment variables.

alamb · 2023-04-03T19:20:13Z

docs/source/user-guide/cli.md


-The CLI can query data in S3 if the following environment variables are defined:
+S3 data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.


Perhaps we should define S3 by adding a link:

Suggested change

S3 data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.

[AWS S3](https://aws.amazon.com/s3/) data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.

alamb · 2023-04-03T19:20:45Z

docs/source/user-guide/cli.md

-```bash
-$ aws s3 cp test.csv s3://my-bucket/
-upload: ./test.csv to s3://my-bucket/test.csv
+OSS data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.


Suggested change

OSS data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.

[Alibaba cloud OSS](https://www.alibabacloud.com/product/object-storage-service) data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.

alamb · 2023-04-03T19:21:21Z

docs/source/user-guide/cli.md

+
+## Registering GCS Data Sources
+
+GCS data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.


Suggested change

GCS data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.

[Google Cloud Storage](https://cloud.google.com/storage) data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.

alamb · 2023-04-03T19:22:32Z

datafusion-cli/src/object_storage.rs

-}
-
-#[cfg(test)]
-mod tests {


do you think there is any value to porting these tests?

alamb · 2023-04-03T19:23:25Z

datafusion-cli/src/object_storage.rs

@@ -1,193 +0,0 @@
-// Licensed to the Apache Software Foundation (ASF) under one


Very nice that we can consolidate the usage

r4ntix · 2023-04-04T03:07:48Z

Thanks @r4ntix -- I think this looks good to me. Thank you for moving the code.

I had some small documentation suggestions.

My biggest concern / question is about testing -- specifically do you think it is worth writing tests (especially for error conditions) for options? I am mostly worried about potentially breaking this functionality with some future refactor but not catching that because of lack fo test coverage

@alamb Thank you very much for your very detailed advice!

About tests, I will add Unit tests for this first. Secondly, I am currently considering whether we can add Integration Tests, e.g. emulator tests of arrow-rs/object_store, but this may take extra time.

alamb · 2023-04-04T10:16:10Z

About tests, I will add Unit tests for this first. Secondly, I am currently considering whether we can add Integration Tests, e.g. emulator tests of arrow-rs/object_store, but this may take extra time.

I agree integration tests would be great, but also that they would take non trivial time to implement and maintain

Unit tests that verify we at least setup the object store correctly I think would be sufficient initially

r4ntix · 2023-04-04T11:44:01Z

I agree integration tests would be great, but also that they would take non trivial time to implement and maintain

Unit tests that verify we at least setup the object store correctly I think would be sufficient initially

Agree, I will add the unit tests as soon as possible.

…tests

alamb · 2023-04-05T18:59:10Z

the CI test https://github.com/apache/arrow-datafusion/actions/runs/4617873525/jobs/8164772317?pr=5732 seems to have failed due to environmental reasons. I have restarted it

alamb · 2023-04-05T19:01:00Z

datafusion-cli/src/exec.rs

+        let err = create_external_table_test(location, &sql)
+            .await
+            .unwrap_err();
+        assert!(err.to_string().contains("No RSA key found in pem file"));


💯 for negative tests

feat: support to obtain necessary information when creating an object…

fbbaacd

… store table through ListingTableFactory

github-actions bot added the core Core DataFusion crate label Mar 25, 2023

r4ntix added 4 commits March 25, 2023 13:34

fix: enable "aws" feature of object_store

734496e

fix: format Cargo.toml

01a7052

chore: update cli docs

7e660b5

fix: format cli.md

b2d2730

r4ntix marked this pull request as ready for review March 25, 2023 12:31

yjshen reviewed Mar 26, 2023

View reviewed changes

alamb reviewed Mar 28, 2023

View reviewed changes

r4ntix added 3 commits April 1, 2023 16:57

chore: move code for registering the object store dynamically out of …

7740c48

…core and into datafusion-cli

chore: improve documentation of cli

6c17bbb

chore: fix typos in doc

d42e98a

alamb approved these changes Apr 3, 2023

View reviewed changes

r4ntix added 3 commits April 5, 2023 18:05

chore: isolate the object store builder to a separate file, add unit …

f1e2d82

…tests

Merge branch 'main' into issue-5731

b07da5f

chore: improve docs

c28fe4b

r4ntix requested a review from alamb April 5, 2023 15:16

alamb reviewed Apr 5, 2023

View reviewed changes

alamb mentioned this pull request Apr 5, 2023

object_store: Builders (S3/Azure/GCS) are missing the get method to get the actual configuration information apache/arrow-rs#4021

Closed

alamb approved these changes Apr 5, 2023

View reviewed changes

alamb merged commit 513f78b into apache:main Apr 5, 2023

r4ntix deleted the issue-5731 branch April 6, 2023 04:20

This was referenced Jun 7, 2023

Add tests for object_store builder of datafusion-cli #6574

Closed

Add tests for object_store builders of datafusion-cli #6576

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support create object store source tables without depending on environment variables #5732

Support create object store source tables without depending on environment variables #5732

r4ntix commented Mar 25, 2023

yjshen Mar 26, 2023

r4ntix Mar 26, 2023

alamb Mar 28, 2023

alamb left a comment

alamb Mar 28, 2023

alamb Mar 28, 2023

r4ntix Mar 29, 2023

r4ntix Mar 29, 2023

alamb Mar 30, 2023

r4ntix Mar 31, 2023

r4ntix commented Apr 1, 2023

alamb left a comment

alamb Apr 3, 2023

alamb Apr 3, 2023

alamb Apr 3, 2023

alamb Apr 3, 2023

alamb Apr 3, 2023

alamb Apr 3, 2023

r4ntix commented Apr 4, 2023

alamb commented Apr 4, 2023

r4ntix commented Apr 4, 2023

alamb commented Apr 5, 2023

alamb Apr 5, 2023

	It is also simplify sql statements by environment variables.
	It is also possible to simplify sql statements by environment variables.


		The CLI can query data in S3 if the following environment variables are defined:
		S3 data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.

	S3 data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.
	[AWS S3](https://aws.amazon.com/s3/) data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.

	OSS data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.
	[Alibaba cloud OSS](https://www.alibabacloud.com/product/object-storage-service) data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.


		## Registering GCS Data Sources

		GCS data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.

	GCS data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.
	[Google Cloud Storage](https://cloud.google.com/storage) data sources can be registered by executing a `CREATE EXTERNAL TABLE` SQL statement.

		@@ -1,193 +0,0 @@
		// Licensed to the Apache Software Foundation (ASF) under one

Support create object store source tables without depending on environment variables #5732

Support create object store source tables without depending on environment variables #5732

Conversation

r4ntix commented Mar 25, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r4ntix commented Apr 1, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r4ntix commented Apr 4, 2023

alamb commented Apr 4, 2023

r4ntix commented Apr 4, 2023

alamb commented Apr 5, 2023

Choose a reason for hiding this comment