Make preserve_identifier_case a three-state setting
Project: duckdb/duckdb | Language: C++ | Concepts: backwards-compatibility, api-design, refactoring | Difficulty: mid
Link: https://github.com/duckdb/duckdb/pull/24375
The problem
DuckDB's preserve_identifier_case setting only allowed a boolean: either preserve the case of unquoted identifiers or always lowercase them. This limited compatibility with systems (like Oracle or DB2) that expect unquoted identifiers to be folded to uppercase. There was also confusion around quoted/single-quoted identifiers and some bugs in identifier folding.
Why it's non-trivial
A naive fix might just add a new boolean or parameter for uppercasing, but that could break backward compatibility, confuse users, or mishandle quoted identifiers and path literals. There are also subtle bugs that can arise if identifier folding is applied in the wrong contexts (e.g., to already-quoted identifiers or file paths).
The approach
1. Introduce a three-state enum for identifier folding
+//! How non-quoted identifiers are folded by the parser
+enum class IdentifierCaseMode : uint8_t { PRESERVE_CASE, LOWERCASE, UPPERCASE };
A new IdentifierCaseMode enum replaces the old boolean, supporting PRESERVE_CASE, LOWERCASE, and UPPERCASE.
2. Update settings registration and default
- "type": "BOOLEAN",
- "default_value": "true"
+ "type": "ENUM<IdentifierCaseMode>",
+ "default_value": "preserve_case",
+ "custom_implementation": [
+ "set"
+ ]
The setting now uses the enum type and defaults to preserve_case. A custom setter is added for input handling.
3. Backward compatibility for boolean inputs
+ // backwards compatibility with the 1.x boolean setting: accept anything that casts to BOOLEAN,
+ // mapping true to preserve_case and false to lowercase
+ auto boolean_value = input.DefaultTryCastAs(LogicalType::BOOLEAN);
+ if (boolean_value) {
+ input = Value(BooleanValue::Get(*boolean_value) ? "preserve_case" : "lowercase");
+ return;
+ }
The setter still accepts old boolean values (true, false, 1, 0, yes, no) and maps them to the appropriate enum value.
4. Parser and matcher refactor to use the new enum
- bool preserve_identifier_case = true;
+ IdentifierCaseMode identifier_case_mode = IdentifierCaseMode::PRESERVE_CASE;
Parser options now use the enum.
- } else if (!state.preserve_identifier_case) {
- result_text = StringUtil::Lower(result_text);
+ } else {
+ state.FoldIdentifier(result_text);
}
Identifier folding is now handled by a new method that applies the correct folding based on the current mode.
5. Centralized folding logic
+ void FoldIdentifier(string &text) const {
+ switch (identifier_case_mode) {
+ case IdentifierCaseMode::LOWERCASE:
+ text = StringUtil::Lower(text);
+ break;
+ case IdentifierCaseMode::UPPERCASE:
+ text = StringUtil::Upper(text);
+ break;
+ default:
+ break;
+ }
+ }
This method ensures that folding is only applied to non-quoted identifiers and according to the selected mode.
6. Fix bugs with quoted identifiers and paths
- } else if (!state.preserve_identifier_case) {
- result_text = StringUtil::Lower(result_text);
- }
- if (IsSingleQuoted(result_text) && SupportsStringLiteral()) {
+ } else if (IsSingleQuoted(result_text) && SupportsStringLiteral()) {
+ // a single-quoted token in a table or file-name position is a path, so it is unwrapped but never folded
result_text = result_text.substr(1, result_text.size() - 2);
result_text = StringUtil::Replace(result_text, "''", "'");
+ } else {
+ state.FoldIdentifier(result_text);
}
Single-quoted path literals are no longer incorrectly folded.
7. Make uppercasing locale-independent
- transform(copy.begin(), copy.end(), copy.begin(), [](unsigned char c) { return std::toupper(c); });
+ transform(copy.begin(), copy.end(), copy.begin(),
+ [](unsigned char c) { return StringUtil::CharacterToUpper(static_cast<char>(c)); });
Uppercasing is now done in a locale-independent way, avoiding issues with non-ASCII characters.
From the review thread
lnkuiper: Looks good to me, just one question for @Mytherin do you think we should have backwards compatible:
true → preserve_case, false → lowercaseOr is that not necessary?
Mytherin: Yes I think we should stay backwards compatible there.
Mytherin: This is always doing string copies even if
IdentifierCaseModeis not set - can we make this take astring &textinstead and do the modification in place when case mode isLOWERCASEorUPPERCASE?
dataders: done in d5a05d3ff9b0f503a2ede383c30d5f6478f68cb7!
Takeaways
- When evolving a configuration setting, preserve backward compatibility by accepting legacy input forms and mapping them to new values.
- Centralize logic for conditional behavior (like identifier folding) to avoid subtle bugs and code duplication.
- Always handle quoted and special-case inputs (like file paths) distinctly from regular identifiers to prevent incorrect transformations.
- Avoid locale-dependent string operations in core logic to ensure consistent behavior across environments.