fix: cases sensitivity for statement options keywords

[seve-martinez]

Closes #2199 .

Prevents casing issues when using the CLI by forcing statement option keys to be lowercase:

smartinez in ~/src/glaredb on smartinez/location_casing_error λ cargo run --bin glaredb local -q "CrEaTe ExTeRnAl TaBlE supplier fRoM lOcAl (LoCaTiOn 'testdata/parquet/userdata1.parquet')"

Table created

[seve-martinez]

@universalmind303 I had originally made this change in the parser logic, but that caused failures in the case-sensitive slt checks. Is #2199 only specific to CLi-based inputs?

[universalmind303]

@universalmind303 I had originally made this change in the parser logic, but that caused failures in the case-sensitive slt checks. Is #2199 only specific to CLi-based inputs?

This should definitely happen in the parser only for the location keyword. If we lowercase the entire query, it'll result in some undesired results

select * from "MyTable" should not be evaluated the same as select * from "mytable". SQL is not case sensitive on keywords, but identifiers are allowed to be case sensitive. the sqlparser crate already handles all of the non custom keywords, so we should just need to handle the parsing of the location keyword.

[seve-martinez]

@universalmind303 I had originally made this change in the parser logic, but that caused failures in the case-sensitive slt checks. Is #2199 only specific to CLi-based inputs?

This should definitely happen in the parser only for the location keyword. If we lowercase the entire query, it'll result in some undesired results

select * from "MyTable" should not be evaluated the same as select * from "mytable". SQL is not case sensitive on keywords, but identifiers are allowed to be case sensitive. the sqlparser crate already handles all of the non custom keywords, so we should just need to handle the parsing of the location keyword.

If i'm following the logic correctly, location is a statement option that's acquired using remove_required or remove_optional:

glaredb/crates/sqlexec/src/planner/session_planner.rs

Lines 596 to 599 in 6e2cb0f

	let account = m.remove_required_or("account_name", account)?;
	let access_key = m.remove_required_or("access_key", access_key)?;
	let location: String = m.remove_required("location")?;

glaredb/crates/sqlexec/src/planner/session_planner.rs

Line 1563 in 6e2cb0f

None => m.remove_required("location")?,

glaredb/crates/sqlexec/src/planner/session_planner.rs

Lines 2052 to 2055 in 6e2cb0f

	let host = m.remove_required("host")?;
	let port = m.remove_optional("port")?;
	let user = m.remove_required("user")?;
	let password = m.remove_optional("password")?;

I did not see any calls to those methods that didn't use a lowercase key, so in that case, would it be best to handle this in remove_required/optional method on StmtOpts?

glaredb/crates/sqlexec/src/parser/options.rs

Line 277 in a2d1c5f

pub fn remove_required<T>(&mut self, k: &str) -> Result<T, ParserError>

Or is it as simple as just checking whether this identifier is location? :

glaredb/crates/sqlexec/src/parser.rs

Line 848 in a2d1c5f

let key = self.parser.parse_identifier()?.value;

[universalmind303]

I think it'd be safe to assume all keys inside options should be case insensitive. I'd expect it to follow SQL semantics where a key of location would be case insensitive, but a key of "Location" would preserve it's casing. Since we don't support quoted keys inside options anyways, I wouldn't worry too much about the latter case for now.

I think we can just .to_lowercase() all of the keys before inserting into the object map.

[universalmind303]

@seve-martinez PR looks good, can we add a slt (sql logic tests) for various casings?

[seve-martinez]

@seve-martinez PR looks good, can we add a slt (sql logic tests) for various casings?

I added some to the external_table.slt file. Let me know if those look okay.

[tychoish]

• the title of the pr seems like it should be updated as it's really all option keywords
• are there other or additional tests that might be useful (I think we're good, but I wanted to check).