Add configurable normalization for configuration options and preserve case for S3 paths

[blaginin]

Which issue does this PR close?

Closes #11650
Related to #13456

Rationale for this change

Surprisingly, even the simplest S3 example doesn't work 😱

> CREATE EXTERNAL TABLE test
STORED AS CSV
OPTIONS(
    'aws.access_key_id' '************',
    'aws.secret_access_key' '**********',
    'aws.region' 'eu-west-1'
)
LOCATION 's3://blaginin/playground/titanic.csv';


Object Store error: The operation lacked the necessary privileges to complete for path playground/titanic.csv: Client error with status 403 Forbidden: No Body

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, added transformation for some fields where it makes sense (enums, bools, etc)

Are these changes tested?

I updated the tests

Are there any user-facing changes?

Yes, changed the default value

[blaginin]

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 has_header would be case-insensitive. Otherwise this doesn't work:

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

[findepi]

This feels the right direction.
Can you add tests showing why this matters?
Perhaps some tests with parsing non-lowercase s3 paths?

[findepi]

you should specify whether a value is case-sensitive per item.

i agree

but it doesn't have to be part of the SQL parser at all.
It can simply be applied when pulling values from the option map (the moment when strings are interpreted into something meaningful)

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

[berkaysynnada]

When I see this PR, I remember a previous discussion: #11650. I have also tried some approaches for it earlier: #11330 (comment).

Isn't it better to implement a future-proof and general solution, like the one I proposed, instead of toggling the configuration up and down?

Do you have time to review the proposed approach and its intended behavior? Is it feasible to implement it without much effort? I have forgotten some of the implementation details, so perhaps you can provide a more reasonable perspective.

[blaginin]

hey @berkaysynnada, by future-proof solution do you mean synnada-ai@341b484#diff-2f697c310af1a48521829d8bd665cf64b6415fbf88edd370efa30b1ed686354dR1655? I agree that fine-grained control is better, but isn't it similar to how I implemented it? e.g.

datafusion/datafusion/common/src/config.rs

Line 442 in 06c013d

pub compression: Option<String>, transform = str::to_lowercase, default = Some("zstd(3)".into())

[findepi]

this changes config option handling.
cc @alamb @jayzhan211 @comphead @crepererum @jonahgao if you want to skim the PR

[comphead]

Thanks @blaginin and @findepi for the review.

[berkaysynnada]

hey @berkaysynnada, by future-proof solution do you mean synnada-ai@341b484#diff-2f697c310af1a48521829d8bd665cf64b6415fbf88edd370efa30b1ed686354dR1655? I agree that fine-grained control is better, but isn't it similar to how I implemented it? e.g.

datafusion/datafusion/common/src/config.rs

Line 442 in 06c013d

pub compression: Option<String>, transform = str::to_lowercase, default = Some("zstd(3)".into())

Sorry, I have missed your reply. After looking more in detail, you did actually what I referred.
I just wonder can we remove that config setting at all 🤔 because now every option can modify the input as intended, and the users should write knowing that behavior -- for example an option is case sensitive, you would know that and write accordingly. But if it's not a specific option and just takes some elements from a set, and then you would know they are all normalized. In which scenarios does that config setting get a meaning?

We have talked with @ozankabak, and the most optimal solution is that we can accommodate this config setting such that it will enables/disables all kind of normalizations. For example, if normalization is disabled, then all given inputs are needed to be written as how they are exactly written. If it is enabled, then the hardcoded normalization functions become activated, and the inputs are normalized accordingly. With all of these,I think the default value should be true, "enable normalizations".

[findepi]

I feel we should deprecate and remove the global config, in favor of per-value normalization.
@berkaysynnada What's the use-case to have per-value normalization and a global config?

[berkaysynnada]

I feel we should deprecate and remove the global config, in favor of per-value normalization. @berkaysynnada What's the use-case to have per-value normalization and a global config?

I know it will most probably be a super-rare use case, but assume we add a hardcoded normalization for an option, and user somehow needs to not normalize his input. Does he need to change the code?

[findepi]

If the option has hard-coded normalization (eg lowercasing), it means there is no difference between certain values (eg those differing in case). Why would a user be concerned about that?
It feels you didn't describe a use-case fully, or I didn't quite understand it.

[berkaysynnada]

If the option has hard-coded normalization (eg lowercasing), it means there is no difference between certain values (eg those differing in case). Why would a user be concerned about that? It feels you didn't describe a use-case fully, or I didn't quite understand it.

If an option accepts a file name and is coded to normalize the input to snake_case (which applies to most data file naming conventions), but for a specific file type snake_case normalization is not preferable, the user might want to retain the original naming style. Can that be a valid example?

I am not insisting that we should keep it, I am just playing devil's advocate 😅

[findepi]

I like this example, thank you @berkaysynnada.
If an option accepts a file name (eg to write data to), would we want to normalize it? Maybe we just shouldn't.

[ozankabak]

The config option is basically an escape hatch for users in case they find themselves in a situation where the normalization we enforce is inapplicable in their (possibly edge) case.

[blaginin]

Feels like the right move is to add a deprecation to that option, see if we get any requests or issues, and if not, just remove it completely.

I also don’t think this option should exist. Each value is either case-sensitive or not - we know that for sure for each value. If we’re missing some edge case, we should fix it instead of normalizing the value. Normalizing might solve one problem but could also break other stuff, like aws integration.

If you’re good with that, @ozankabak @berkaysynnada, I’ll deprecate the option

[ozankabak]

I am OK with removing it. We can add it later if it turns out to be necessary

[blaginin]

> set datafusion.sql_parser.enable_options_value_normalization to true;
[2024-12-13T12:44:10Z WARN  datafusion_common::config] `enable_options_value_normalization` is deprecated and ignored

[alamb]

Thanks @blaginin

This seems like a very nice improvement to me and moves the choice of when to lowercase the options to the individual options

@ozankabak / @berkaysynnada can you confirm the change of behavior in this PR will work for your usecase?

[alamb]

the use of the warning is quite cool

However, it isn't shown by default in datafusion-cli FWIW

> set datafusion.sql_parser.enable_options_value_normalization = true;
0 row(s) fetched.
Elapsed 0.001 seconds.

I had to run it with RUST_LOG=info

RUST_LOG=info cargo run
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.13s
     Running `target/debug/datafusion-cli`
DataFusion CLI v43.0.0
> set datafusion.sql_parser.enable_options_value_normalization = true;
[2024-12-19T23:23:40Z WARN  datafusion_common::config] `enable_options_value_normalization` is deprecated and ignored
0 row(s) fetched.
Elapsed 0.007 seconds

[alamb]

Could you please put this test back and updated it so that it passes? That way we can reason about / more easily see the change in behavior introduced by this PR

[alamb]

Thank you @findepi for the reviews

[berkaysynnada]

Thanks @blaginin

This seems like a very nice improvement to me and moves the choice of when to lowercase the options to the individual options

@ozankabak / @berkaysynnada can you confirm the change of behavior in this PR will work for your usecase?

Yes, we can merge this. If additional needs arise, we can request/submit follow-up PRs.

[ozankabak]

🚀 🚀

[alamb]

Thanks @blaginin @findepi @comphead @berkaysynnada and @ozankabak for sticking with this. It is a nice improvement.