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

allow datafusion cli to take -- comments #296

Merged
merged 2 commits into from
May 9, 2021

Conversation

jimexist
Copy link
Member

@jimexist jimexist commented May 9, 2021

Which issue does this PR close?

Closes #297.

Rationale for this change

-- comments shall be respected in parsing command line input

What changes are included in this PR?

  • adding support for -- for both file and cli mode

Are there any user-facing changes?

If there are user-facing changes then we may require documentation to be updated before approving the PR.

If there are any breaking changes to public APIs, please add the breaking change label.

@jimexist
Copy link
Member Author

jimexist commented May 9, 2021

❯ cargo run --release --bin datafusion-cli -q
> -- hello
> select 1 num;
+-----+
| num |
+-----+
| 1   |
+-----+
1 rows in set. Query took 0 seconds.

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2021

Codecov Report

Merging #296 (5e1b074) into master (204d4f5) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #296      +/-   ##
==========================================
- Coverage   75.99%   75.98%   -0.01%     
==========================================
  Files         141      141              
  Lines       23657    23659       +2     
==========================================
  Hits        17978    17978              
- Misses       5679     5681       +2     
Impacted Files Coverage Δ
datafusion-cli/src/main.rs 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 204d4f5...5e1b074. Read the comment docs.

Copy link
Contributor

@msathis msathis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

Thanks @jimexist ; nice and sweet.

Copy link
Contributor

@Dandandan Dandandan left a comment

Choose a reason for hiding this comment

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

I am wondering, can't we fix the code so comments are parsed correctly rather than skipping them?
I believe you might be able to change the code to add new lines rather than a space at the end of every line.

query.push(' ');
// To
query.push('\n');

This way we should also parse multiline comments, comments starting with a space, etc correctly.

@jimexist
Copy link
Member Author

jimexist commented May 9, 2021

I am wondering, can't we fix the code so comments are parsed correctly rather than skipping them?
I believe you might be able to change the code to add new lines rather than a space at the end of every line.

query.push(' ');
// To
query.push('\n');

This way we should also parse multiline comments, comments starting with a space, etc correctly.

@Dandandan thanks for the advise. i've updated the appending to use \n instead.

However I still think we shall skip -- starting lines like the changes i made here, otherwise a line with -- ; will result in an error

@jimexist jimexist force-pushed the fix-cli-comment branch from 2dd4c5d to 5e1b074 Compare May 9, 2021 15:07
@Dandandan
Copy link
Contributor

I am wondering, can't we fix the code so comments are parsed correctly rather than skipping them?
I believe you might be able to change the code to add new lines rather than a space at the end of every line.

query.push(' ');
// To
query.push('\n');

This way we should also parse multiline comments, comments starting with a space, etc correctly.

@Dandandan thanks for the advise. i've updated the appending to use \n instead.

However I still think we shall skip -- starting lines like the changes i made here, otherwise a line with -- ; will result in an error

Fair enough, thanks! I think at some point we should remove the ; check and work together with the parser to detect true statement endings. Untill then this seems a good compromise.

@jimexist
Copy link
Member Author

jimexist commented May 9, 2021

Agreed that this is a compromise - at least writing multiple semicolons will still break A

@Dandandan Dandandan merged commit a00f410 into apache:master May 9, 2021
@jimexist jimexist deleted the fix-cli-comment branch May 10, 2021 01:00
@houqp houqp added datafusion Changes in the datafusion crate enhancement New feature or request labels Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datafusion Changes in the datafusion crate enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datafusion cli should support -- line comments
6 participants