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

Support #$ query interpolation #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rbanikaz
Copy link

#20

@matthew-lucidchart
Copy link
Contributor

Has this been benchmarked?

@pauldraper
Copy link
Contributor

@matthew-lucidchart, we do need to fix relate-benchmarks.

They don't work out-of-the-box, and currently there are no instructions on getting them to work. And though there are a lot of tests, a lot of them are never run (e.g. we only ever parameterize with ints, nothing else).

I'm agreed that this (and all other changes) should be benchmarked.

@matthew-lucidchart
Copy link
Contributor

Yeah, it's too bad we didn't keep up on them, but their primary purpose was
to make sure we were headed down the right path from the start.

Simpler benchmarking can be done without that project.

On Fri, Mar 13, 2015 at 1:35 AM, Paul Draper [email protected]
wrote:

@matthew-lucidchart https://github.com/matthew-lucidchart, we do need
to fix relate-benchmarks.

They don't work out-of-the-box, and currently there are no instructions on
getting them to work. And though there are a lot of tests, a lot of them
are never run (e.g. we only ever parameterize with ints, nothing else).

I'm agreed that this (and all other changes) should be benchmarked.


Reply to this email directly or view it on GitHub
#21 (comment).

@rbanikaz
Copy link
Author

Please let me know if anything else I need to get this merged?

@matthew-lucidchart
Copy link
Contributor

Benchmarking them would go a long way in my view. Either via relate-benchmarks or your own make.

@gregghz
Copy link
Collaborator

gregghz commented Feb 3, 2016

using #$ appears to be quite a bit faster than using .toSql.

interpolation

The left is:

val s = "SELECT".toSql
val f = "FROM".toSql
val w = "WHERE".toSql
sql"$s * $f table $w 1 = 1"

The right is:

val s = "SELECT"
val f = "FROM"
val w = "WHERE"
sql"#$s * #$f table #$w 1 = 1"

@lucidchart
Copy link

what about:

sql"${"SELECT".toSQL} * ${"FROM".toSQL} ......

On Wed, Feb 3, 2016 at 2:44 PM, Gregg Hernandez [email protected]
wrote:

using #$ appears to be quite a bit faster than using .toSql.

[image: interpolation]
https://camo.githubusercontent.com/5919e91b40976c3df9e45e7993e98b5666248594/687474703a2f2f692e696d6775722e636f6d2f734c35525a6e392e706e67

The left is:

val s = "SELECT".toSql
val f = "FROM".toSql
val w = "WHERE".toSql
sql"$s * $f table $w 1 = 1"

The right is:

val s = "SELECT"
val f = "FROM"
val w = "WHERE"
sql"#$s * #$f table #$w 1 = 1"


Reply to this email directly or view it on GitHub
#21 (comment).

@gregghz
Copy link
Collaborator

gregghz commented Feb 3, 2016

what lucidchart said is the same as the original toSql benchmark.

@gregghz
Copy link
Collaborator

gregghz commented Feb 4, 2016

We should probably have some unit tests on this that check to make sure the output is what we expect.

@gregghz
Copy link
Collaborator

gregghz commented Aug 3, 2016

@rbanikaz If you're still interested in getting this merged, you'll need to update the PR (merge conflicts) and please include some unit tests as well.

@tmccombs
Copy link
Contributor

I'm inclined to reject this, because it can encourage writing code that is vulnerable to SQL injection. That said, I think having better syntax for thing like table and column names would be a good thing.

I think the proper way to do this would be to escape the name as an identifier before interpolating it. So for example, in mysql the string "foo bar" would be interpolated as `foo bar`.

Note that the escaping is different in different SQL implementations, and the quoting character needs to be escaped.

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

Successfully merging this pull request may close these issues.

6 participants