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

Fix SQL planner to support multibyte column names #357

Merged
merged 3 commits into from
May 19, 2021
Merged

Conversation

agatan
Copy link
Contributor

@agatan agatan commented May 18, 2021

Which issue does this PR close?

Closes #356

Rationale for this change

Please refer #356 for more detail.

What changes are included in this PR?

Slicing string will cause panic if the boundary is not correct as UTF-8.
To check if the first character is '@' or not, we should use s.chars().nth(0) instead of &s[0..1].

Are there any user-facing changes?

No.

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.

LGTM! 👍

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.

😀 emoji support! 💯

@agatan
Copy link
Contributor Author

agatan commented May 19, 2021

Oh, sorry, I made a mistake in the test case.
I'll fix it now.

@alamb
Copy link
Contributor

alamb commented May 19, 2021

Looks great @agatan - thank you! I just started CI on this PR and once we get a clean run I think this one is ready to merge

@codecov-commenter
Copy link

codecov-commenter commented May 19, 2021

Codecov Report

Merging #357 (be821d5) into master (6c050b8) will increase coverage by 0.12%.
The diff coverage is 97.50%.

❗ Current head be821d5 differs from pull request most recent head a5df543. Consider uploading reports for the commit a5df543 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #357      +/-   ##
==========================================
+ Coverage   75.72%   75.85%   +0.12%     
==========================================
  Files         143      143              
  Lines       23910    23814      -96     
==========================================
- Hits        18107    18064      -43     
+ Misses       5803     5750      -53     
Impacted Files Coverage Δ
datafusion/src/physical_plan/functions.rs 92.77% <97.05%> (+3.19%) ⬆️
datafusion/src/sql/planner.rs 83.38% <100.00%> (+0.06%) ⬆️

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 6c050b8...a5df543. Read the comment docs.

@alamb
Copy link
Contributor

alamb commented May 19, 2021

@agatan can you please fix the clippy error: https://github.com/apache/arrow-datafusion/pull/357/checks?check_run_id=2620378740


To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: called `.nth(0)` on a `std::iter::Iterator`, when `.next()` is equivalent
   --> datafusion/src/sql/planner.rs:860:20
    |
860 |                 if id.value.chars().nth(0) == Some('@') {
    |                    ^^^^^^^^^^^^^^^^^^^^^^^ help: try calling `.next()` instead of `.nth(0)`: `id.value.chars().next()`
    |
    = note: `-D clippy::iter-nth-zero` implied by `-D warnings`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero

@agatan
Copy link
Contributor Author

agatan commented May 19, 2021

Thank you! I just fixed it now.

@alamb alamb merged commit 5c5a0bb into apache:master May 19, 2021
@alamb alamb added bug Something isn't working datafusion Changes in the datafusion crate labels May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working datafusion Changes in the datafusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL Planner panics when multibyte characters are used in column names
6 participants