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

Remove AS from table aliases. #4835

Merged
merged 2 commits into from
Oct 11, 2014
Merged

Remove AS from table aliases. #4835

merged 2 commits into from
Oct 11, 2014

Conversation

markstory
Copy link
Member

Having AS causes issues with Oracle. While this is not a core driver, the AS keyword is not required by any supported driver.

Refs #4830

Having AS causes issues with Oracle. While this is not a core driver,
the AS keyword is not required by any supported driver.

Refs #4830
@thaJeztah
Copy link

Oracle bad :(

Could this be made dependent on the driver used? I find queries without the AS keyword a lot harder to read. I know it's optional, just more verbose and easier to read 😄

@markstory
Copy link
Member Author

Well drivers can implement their own QueryCompiler. Theoretically, not including AS will make the ORM a teeny tiny bit faster as there are fewer bytes to allocate in memory, and send to the db server.

@thaJeztah
Copy link

We're entering pico-optimisation territory here!

Generally, I'm ok-ish with this change, because it's simple. However, because Oracle is not a "core" driver (and Oracle is just stubborn to not comply with the SQL standards), I would prefer having this exception in the Oracle driver, if possible.

@ravage84
Copy link
Member

ravage84 commented Oct 9, 2014

@markstory Having theAS makes it the SQL a bit more readable on the other hand 😼

@thaJeztah SQL standards, don't make me laugh 🍭

@thaJeztah
Copy link

@ravage84 agreed, most databases don't fully comply with the "standards" (and then again, which version) and many extend on those "standards", but not supporting something very basic as AS is (in my opinion) just being stubborn.

Having said that, my main issue here is the readability of the queries, so on that point, I think we're in agreement 😄

@rchavik
Copy link
Member

rchavik commented Oct 9, 2014

Having a supported Oracle driver is better. So I don't mind this.

@ADmad
Copy link
Member

ADmad commented Oct 10, 2014

Sqlserver has couple of failing tests

@htstudios
Copy link
Contributor

I like it for readability too, but nevermind.

markstory added a commit that referenced this pull request Oct 11, 2014
Remove AS from table aliases.
@markstory markstory merged commit 797ce52 into 3.0 Oct 11, 2014
@markstory markstory deleted the issue-4830 branch October 11, 2014 12:24
@markstory
Copy link
Member Author

Merging, bit we can always revisit this in the future if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants