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

Introduce \i command to execute from a file #3136

Merged
merged 2 commits into from
Aug 17, 2022

Conversation

turbo1912
Copy link
Contributor

@turbo1912 turbo1912 commented Aug 14, 2022

Which issue does this PR close?

Closes #1906.

Rationale for this change

(from the original issue):
Taking inspiration frompsql , it implements a \i command (not a command line argument) for this usecase. It includes a file:

https://www.postgresql.org/docs/13/app-psql.html

\i or \include filename
Reads input from the file filename and executes it as though it had been typed on the keyboard.

@turbo1912 turbo1912 changed the title Introduce \i command to include files Introduce \i command to execute from a file Aug 14, 2022
@codecov-commenter
Copy link

codecov-commenter commented Aug 14, 2022

Codecov Report

Merging #3136 (73b57fe) into master (48f9b7a) will decrease coverage by 0.07%.
The diff coverage is 76.53%.

❗ Current head 73b57fe differs from pull request most recent head 982e72b. Consider uploading reports for the commit 982e72b to get more accurate results

@@            Coverage Diff             @@
##           master    #3136      +/-   ##
==========================================
- Coverage   85.95%   85.88%   -0.08%     
==========================================
  Files         291      291              
  Lines       52382    52758     +376     
==========================================
+ Hits        45025    45309     +284     
- Misses       7357     7449      +92     
Impacted Files Coverage Δ
...afusion/core/src/datasource/file_format/parquet.rs 85.57% <0.00%> (-0.70%) ⬇️
...ore/src/physical_plan/file_format/chunked_store.rs 62.26% <0.00%> (-5.09%) ⬇️
datafusion/core/src/physical_plan/mod.rs 88.00% <ø> (ø)
datafusion/core/tests/path_partition.rs 84.92% <0.00%> (-0.87%) ⬇️
datafusion/physical-expr/src/planner.rs 92.36% <ø> (ø)
...on/physical-expr/src/expressions/binary/kernels.rs 52.17% <44.44%> (-10.99%) ⬇️
datafusion/common/src/scalar.rs 84.61% <63.15%> (-0.28%) ⬇️
...sion/core/src/physical_plan/file_format/parquet.rs 94.56% <75.00%> (-0.79%) ⬇️
...tafusion/physical-expr/src/expressions/datetime.rs 84.22% <78.60%> (-2.58%) ⬇️
datafusion/sql/src/planner.rs 82.00% <90.00%> (+0.05%) ⬆️
... and 11 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@andygrove
Copy link
Member

Thanks for the contribution @turbo1912. It would be good if we could merge #3133 first then update this PR with the documentation changes.

@@ -72,6 +76,22 @@ impl Command {
.print_batches(&batches, now)
.map_err(|e| DataFusionError::Execution(e.to_string()))
}
Self::Include(filename) => {
if let Some(filename) = filename {
let file = match File::open(filename) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we either use ? or map_err here rather than this match?

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.

I tried it out and it works great! Thanks @turbo1912

❯ \i /tmp/bar.sql
+---------------------+
| Int64(1) + Int64(2) |
+---------------------+
| 3                   |
+---------------------+
(arrow_dev) alamb@MacBook-Pro-8:~/Software/arrow-datafusion/datafusion-cli$ cat /tmp/bar.sql 
select 1+2;

@andygrove not sure if you think we need to update the docs in this PR or if we could do it as a follow on PR

Comment on lines 81 to 85
let file = match File::open(filename) {
Ok(file) => file,
Err(error) => {
return Err(DataFusionError::Execution(error.to_string()))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, something like this perhaps?

Suggested change
let file = match File::open(filename) {
Ok(file) => file,
Err(error) => {
return Err(DataFusionError::Execution(error.to_string()))
}
let file = File::open(filename)
.map_err(|e| {
DataFusionError::Execution(format!("Error opening {:?} {}", filename, e))
})?;

As the e doesn't contain the filename

This gives something like:

❯ \i /tmp/foo.dd
Execution error: Error opening "/tmp/foo.dd" No such file or directory (os error 2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! yup of course, happy to add the documentation change as well :)

@andygrove
Copy link
Member

@andygrove not sure if you think we need to update the docs in this PR or if we could do it as a follow on PR

Either way is fine with me.

@andygrove andygrove merged commit 2aa0a98 into apache:master Aug 17, 2022
@ursabot
Copy link

ursabot commented Aug 17, 2022

Benchmark runs are scheduled for baseline = 23866cd and contender = 2aa0a98. 2aa0a98 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

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.

Add \i command to datafusion-cli
5 participants