-
Notifications
You must be signed in to change notification settings - Fork 1
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
Handle case/quoting when replacing names #34
Comments
Recovering the original formatting I guess is the interesting part, assuming we want that. We'd only go up to a point here, something like:
|
For a v1 perhaps even the following is fine:
|
It seems like whether we want to be case sensitive is not only driver specific, it can get pretty nuanced and difficult for us to determine for some of them. According to ClaudeAI:
For Macaw we can kick this can of worms down the road by taking a flag, and for Metabase we can probably get by for a start by using the default for each driver, and not bothering to detect system variables, connection settings, or handling double quotes. |
That's subtly wrong for Postgres. Quotes preserve case, otherwise things are normalized to lowercase:
@piranha believes that behavior is similar for other DBs we care about but I haven't personally looked into it exhaustively |
Except for mysql, where it doesn't care about casing at all? But for me the only place I had to handle this is tests, actual code stayed the same. This actually depends on how db was created (which collation it uses). |
I've realized that an interesting wrinkle here is that whether the replacement needs to be quoted depends on the replacement name only, not whether the original name was quoted. In order to preserve styling however, it would also be good to preserve quotes when they were present but not necessary on the original. Whether we should remove them when renaming from a name that requires quoting to one that does not, i.e. whether the user likes to quote regardless, like the query processor, may be indeterminable from a small query without other references. To keep things simple for now, we'll only quote when necessary, which saves us carrying over the initial state, but also take an optional |
It turns out that knowing when to quote is even more complicated - in most schema elements are still case sensitive, it's just that non quoted identifiers are treated as implicitly uppercase or lowercase (database and config dependent). So for one database an identifier references a lowercase field needs to be quoted, and for another database it does not. To make matters worse that case sensitivity can even be a property of the table itself that the field is a part of - the collation defined on it in the database. |
Similar to the work done in metabase/metabase#41781
When we call
replace-names
, we should handle case and quoting correctly. Currently it depends on an exact string match.The text was updated successfully, but these errors were encountered: