-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 S3 in CLI: Do not normalize options values #13576
base: main
Are you sure you want to change the base?
Conversation
Actually, I think it should work differently - you should specify whether a value is case-sensitive per item. For example, an AWS password would be case-sensitive, but CREATE EXTERNAL TABLE test
STORED AS CSV
LOCATION 'data.csv'
OPTIONS ('has_header' 'TRUE');
Error parsing TRUE as bool
caused by
External error: provided string was not `true` or `false` Will change |
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.
This feels the right direction.
Can you add tests showing why this matters?
Perhaps some tests with parsing non-lowercase s3 paths?
i agree but it doesn't have to be part of the SQL parser at all. alternatively, we could have some sort of option registry where options could have a type declared, so that parsing is consistent, but that would be a much bigger change |
@@ -362,3 +362,15 @@ OPTIONS ('format.terminator' E'\r', 'format.has_header' 'true'); | |||
|
|||
statement ok | |||
drop table stored_table_with_cr_terminator; | |||
|
|||
|
|||
# check that datetime_format is case sensitive |
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.
that doesn't work, reported a separate issue #13633
Which issue does this PR close?
Related to #13456
Rationale for this change
Surprisingly, even the simplest S3 example doesn't work 😱
That's because s3 keys are case-sensitive, but now we always lowercase them.
I see you had the same issue for HuggingFace so I really think the default behaviour should be NOT normalizing.
What changes are included in this PR?
Change the default value
Are these changes tested?
I updated the tests
Are there any user-facing changes?
Yes, changed the default value