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.

Sign up for GitHub

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

Jump to bottom

bug: Datafusion doesn't respect case sensitive table references #8964

Merged

alamb merged 6 commits into apache:main from xhwhis:table-alias

Jan 26, 2024

Contributor

xhwhis commented Jan 23, 2024 •

edited

Loading

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_str method converts uppercase to lowercase. The alias here has already undergone normalize, so we can directly create a TableReference as a parameter.

Are these changes tested?

Yes

Are there any user-facing changes?

No


          fix: table alias will process uppercase alias into lowercase

96dac98

github-actions bot added the sql label

comphead reviewed

View reviewed changes

Contributor

comphead left a comment

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

xhwhis closed this

xhwhis reopened this


          test: uppercase table alias

f314138

Contributor Author

xhwhis commented Jan 24, 2024 •

edited

Loading

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 datafusion-cli to observe the uppercase bug in table alias.

DataFusion CLI v34.0.0
❯ CREATE TABLE IF NOT EXISTS valuetable(c1 INT, c2 VARCHAR) AS VALUES(1,'HELLO'),(12,'DATAFUSION');
0 rows in set. Query took 0.066 seconds.

❯ SELECT "TEST".c1 from valuetable as "TEST";
Schema error: No field named test.c1. Valid fields are test.c1, test.c2.

I believe this PR have a minimal impact. I have added unit tests to cover the changes. Please review. @comphead

comphead reviewed

View reviewed changes

Contributor

comphead left a comment

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


          test: add table alias sqllogictest

d013f77

github-actions bot added the sqllogictest label

Contributor Author

xhwhis commented Jan 24, 2024

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.

comphead reviewed

View reviewed changes

datafusion/sql/tests/sql_integration.rs Outdated

@@ @@ -4278,6 +4294,23 @@ fn test_table_alias() { @@
                       \n          TableScan: person";
                   quick_test(sql, expected);
+                  let sql = "select * from (\

Contributor

comphead Jan 24, 2024

I think we getting closer, please move those tests to select.rs you may want to use EXPLAIN keyword to get the plan

Contributor Author

xhwhis Jan 25, 2024

Done.


          test: move tests to sqllogictests

bffe6e7

comphead changed the title ~~fix: table alias will process uppercase alias into lowercase~~ bug: Datafusion doesn't respect case sensitive table references

comphead reviewed

View reviewed changes

Contributor

comphead left a comment

Thanks @xhwhis we are really close

datafusion/sqllogictest/test_files/cte.slt Outdated

@@ @@ -19,3 +19,20 @@ query II @@
               select * from (WITH source AS (select 1 as e) SELECT * FROM source) t1,   (WITH source AS (select 1 as e) SELECT * FROM source) t2
               ----
 1
+              query I
+              WITH "T" AS (SELECT 1 a) SELECT * FROM "T"

Contributor

comphead Jan 25, 2024

Suggested change

      
            WITH "T" AS (SELECT 1 a) SELECT * FROM "T"
          
            WITH "T" AS (SELECT 1 a) SELECT "T".* FROM "T"

Contributor Author

xhwhis Jan 25, 2024

Done.


          test: update cte.slt

b5766ef

comphead approved these changes

View reviewed changes

Contributor

comphead left a comment

lgtm thanks @xhwhis
since it is first time contribution I'm leaving it for sometime to let other people review as well

comphead reviewed

View reviewed changes

datafusion/sqllogictest/test_files/cte.slt

@@ @@ -19,3 +19,20 @@ query II @@
               select * from (WITH source AS (select 1 as e) SELECT * FROM source) t1,   (WITH source AS (select 1 as e) SELECT * FROM source) t2
               ----
 1
+              query I

Contributor

comphead Jan 25, 2024

Please add a test description

Contributor

alamb Jan 25, 2024

Suggested change

      
            query I
          
            # Ensure table aliases can be case sensitive
          
            query I

Contributor Author

xhwhis Jan 26, 2024

Done. @alamb @comphead

comphead reviewed

View reviewed changes

datafusion/sqllogictest/test_files/cte.slt

+              ----
+
+              query TT

Contributor

comphead Jan 25, 2024

ditto

alamb approved these changes

View reviewed changes

datafusion/sqllogictest/test_files/cte.slt

@@ @@ -19,3 +19,20 @@ query II @@
               select * from (WITH source AS (select 1 as e) SELECT * FROM source) t1,   (WITH source AS (select 1 as e) SELECT * FROM source) t2
               ----
 1
+              query I

Contributor

alamb Jan 25, 2024

Suggested change

      
            query I
          
            # Ensure table aliases can be case sensitive
          
            query I

datafusion/sqllogictest/test_files/cte.slt

+              ----
+
+              query TT

Contributor

alamb Jan 25, 2024

Suggested change

      
            query TT
          
            # Ensure table aliases can be case sensitive
          
            query TT


          chore: add test descriptions

02b7ea8

alamb approved these changes

View reviewed changes

Contributor

alamb left a comment

Thank you @xhwhis -- this looks great. I plan to merge it after CI passes

alamb merged commit 35c7b2c into apache:main

22 checks passed

Contributor

alamb commented Jan 26, 2024

Thanks again @xhwhis

xhwhis deleted the table-alias branch

January 26, 2024 11:39

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sql sqllogictest