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

ignore case of with header row in sql when creating external table #1237

Merged
merged 3 commits into from
Nov 7, 2021
Merged

ignore case of with header row in sql when creating external table #1237

merged 3 commits into from
Nov 7, 2021

Conversation

dylankyc
Copy link
Contributor

@dylankyc dylankyc commented Nov 4, 2021

Resolves #1242

Currently, creating an external table using datafusion-cli with this sql:

CREATE EXTERNAL TABLE t(c1 int) STORED AS CSV with header row LOCATION 'foo.csv'

will fail when with header row tokens are in lower case. This pr is a fix.

@github-actions github-actions bot added datafusion Changes in the datafusion crate sql SQL Planner labels Nov 4, 2021
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you @lichuan6

@alamb
Copy link
Contributor

alamb commented Nov 4, 2021

@lichuan6 there appears to be some failures on the CI checks

@houqp houqp added the bug Something isn't working label Nov 5, 2021
@dylankyc
Copy link
Contributor Author

dylankyc commented Nov 5, 2021

@lichuan6 there appears to be some failures on the CI checks

OK, thanks @alamb for pointing it out. It seems using self.consume_token(&Token::make_keyword("WITH")) will only recognize upper case keyword. What's the best way to do case-insensitive token parsing?

Currently, after changing signature from

fn consume_token(&mut self, expected: &str) -> bool

to

fn consume_token(&mut self, expected: &Token) -> bool

to do case-insensitive parsing, I created a new Token::Word enum with upper case of next token. Don't know if this is the best practice.

@dylankyc dylankyc requested a review from alamb November 5, 2021 05:36
@dylankyc
Copy link
Contributor Author

dylankyc commented Nov 5, 2021

@Dandandan Could you please approve and make the tests run?

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you @lichuan6

Any thoughts @Dandandan ?

@dylankyc
Copy link
Contributor Author

dylankyc commented Nov 6, 2021

Hi, @Dandandan Could you please review this PR?

@alamb
Copy link
Contributor

alamb commented Nov 6, 2021

If @Dandandan doesn't have anything to add I'll plan to merge this PR tomorrow

@alamb alamb merged commit a8029e5 into apache:master Nov 7, 2021
@alamb
Copy link
Contributor

alamb commented Nov 7, 2021

Thanks again @lichuan6

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 sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CREATE EXTERNAL TABLE ... STORED AS CSV with header row fails when with header row is lowercase
4 participants