-
Notifications
You must be signed in to change notification settings - Fork 1.9k
bug: Datafusion doesn't respect case sensitive table references #8964
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
Conversation
comphead
left a comment
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.
Thanks @xhwhis for your contribution
Would you mind adding more details on reason why this change is need, and would be great to cover the change with unit tests
You can use I believe this PR have a minimal impact. I have added unit tests to cover the changes. Please review. @comphead |
comphead
left a comment
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'm able to reproduce with
❯ select "T".* from (select 1 a) as "T";
Error during planning: Invalid qualifier T
❯ select t.* from (select 1 a) as "T";
+---+
| a |
+---+
| 1 |
+---+
1 row in set. Query took 0.005 seconds.
Please include this test into the test case, and would be nice to move tests into select.slt sqllogic test file
I have added sqllogictests as per your suggestion. |
| \n TableScan: person"; | ||
| quick_test(sql, expected); | ||
|
|
||
| let sql = "select * from (\ |
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 we getting closer, please move those tests to select.rs you may want to use EXPLAIN keyword to get the plan
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.
Done.
comphead
left a comment
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.
Thanks @xhwhis we are really close
| 1 1 | ||
|
|
||
| query I | ||
| WITH "T" AS (SELECT 1 a) SELECT * FROM "T" |
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.
| WITH "T" AS (SELECT 1 a) SELECT * FROM "T" | |
| WITH "T" AS (SELECT 1 a) SELECT "T".* FROM "T" |
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.
Done.
comphead
left a comment
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.
lgtm thanks @xhwhis
since it is first time contribution I'm leaving it for sometime to let other people review as well
| ---- | ||
| 1 1 | ||
|
|
||
| query I |
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.
Please add a test description
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.
| query I | |
| # Ensure table aliases can be case sensitive | |
| query I |
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.
| ---- | ||
| 1 | ||
|
|
||
| query TT |
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.
ditto
| ---- | ||
| 1 1 | ||
|
|
||
| query I |
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.
| query I | |
| # Ensure table aliases can be case sensitive | |
| query I |
| ---- | ||
| 1 | ||
|
|
||
| query TT |
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.
| query TT | |
| # Ensure table aliases can be case sensitive | |
| query TT |
alamb
left a comment
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.
Thank you @xhwhis -- this looks great. I plan to merge it after CI passes
|
Thanks again @xhwhis |
Which issue does this PR close?
Closes #.
Rationale for this change
Table alias will process uppercase alias into lowercase. This PR will fix it.
What changes are included in this PR?
TableReference::parse_strmethod converts uppercase to lowercase. The alias here has already undergonenormalize, so we can directly create a TableReference as a parameter.Are these changes tested?
Yes
Are there any user-facing changes?
No