-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add print format param and support for csv print format to datafusion cli #289
Conversation
|
|
this shall pave the way for finally merging #281 |
Codecov Report
@@ Coverage Diff @@
## master #289 +/- ##
==========================================
- Coverage 76.07% 75.99% -0.09%
==========================================
Files 140 141 +1
Lines 23632 23657 +25
==========================================
Hits 17978 17978
- Misses 5654 5679 +25
Continue to review full report at Codecov.
|
66bcfc4
to
3f3de23
Compare
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.
The feature train 🚋 keeps on rolling! This is great @jimexist . Thank you
let mut writer = builder.build(&file); | ||
batches | ||
.iter() | ||
.for_each(|batch| writer.write(batch).unwrap()); |
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 think it would be good to return the error here rather than calling unwrap()
file.read_to_string(&mut data)?; | ||
println!("{}", data); | ||
} | ||
PrintFormat::Aligned => pretty::print_batches(batches)?, |
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.
FWIW I called this format Pretty
when I was doing something similar in IOx -- I don't think it is really important, however
3f3de23
to
c436afd
Compare
|
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 looks great @jimexist ! 💯💯💯
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.
😍 Looks great. Thanks @jimexist
Which issue does this PR close?
Closes #290.
This shall be merged after #285
Rationale for this change
Printing results in CSV mode allows for better parsing, e.g. in automatic systems.
What changes are included in this PR?
- adding tmpfile deps--format
flagAre there any user-facing changes?