Skip to content
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

Stop creating SQL literals for aliases. #187

Closed
wants to merge 1 commit into from

Conversation

ernie
Copy link
Collaborator

@ernie ernie commented Jun 5, 2013

PostgreSQL requires that the aliases aren't quoted, but MySQL and SQLite
don't seem to care at all, and it seems like we would do better to avoid
creating SqlLiteral nodes where we don't need to.

I ran the mysql/postgresql/sqlite3 tests for ActiveRecord against this
branch and everything passes. Of course, we all know that doesn't
necessarily mean anything.

This is tangentially related to activerecord-hackery/squeel#246, but still doesn't
address the "reserved word" complaint there. Might make sense to have an
Alias node for special visitation vs the SqlLiteral one, perhaps. Or
maybe we just don't care if someone wants to alias things using reserved
words in pg, because that's just cray-cray.

PostgreSQL requires that the aliases aren't quoted, but MySQL and SQLite
don't seem to care at all, and it seems like we would do better to avoid
creating SqlLiteral nodes where we don't need to.

I ran the mysql/postgresql/sqlite3 tests for ActiveRecord against this
branch and everything passes. Of course, we all know that doesn't
necessarily mean anything.

This is tangentially related to activerecord-hackery/squeel#246, but still doesn't
address the "reserved word" complaint there. Might make sense to have an
Alias node for special visitation vs the SqlLiteral one, perhaps. Or
maybe we just don't care if someone wants to alias things using reserved
words in pg, because that's just cray-cray.
@ernie
Copy link
Collaborator Author

ernie commented Jun 5, 2013

@tenderlove Pinging you for thoughts here. Seems like a moderately risky thing to do and I'd rather not be the only eyes on it.

@tenderlove
Copy link
Member

If the other dbs pass, then :shipit:

@ernie
Copy link
Collaborator Author

ernie commented Jun 21, 2013

@tenderlove If, by "other DBs", you mean frontbase, oracle, etc, well then I'm gonna need to ping some other folks, as I don't have them running here. Otherwise, this was verified with MySQL, PostgreSQL, and SQLite3, using rake test.

@matthewd
Copy link
Member

matthewd commented May 4, 2014

@ernie err, PostgreSQL is entirely happy with quoted aliases... they just have to be using "identifier" quotes, not 'string literal' quotes. And without actually going and checking the standard, I bet that's the defined behaviour (and thus the more likely to work on other platforms).

@tamird
Copy link

tamird commented Sep 16, 2014

@ernie, is this still relevant? needs a rebase if so

@kbrock
Copy link

kbrock commented Jan 10, 2018

@ernie Is this still relevant?
Were you thinking of going with " instead of '?
That would avoid the whole "how do I tell the library I actually want to quote this" dilemma.

@matthewd
Copy link
Member

Per #523, Arel development is moving to rails/rails.

If this PR is still relevant, please consider reopening it over there.

@matthewd matthewd closed this Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants