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

Calling parser with backtick #1512

Draft
wants to merge 10 commits into
base: v5
Choose a base branch
from
Draft

Calling parser with backtick #1512

wants to merge 10 commits into from

Conversation

lucioreyli
Copy link

No description provided.

@mathiasrw
Copy link
Member

mathiasrw commented Oct 6, 2022

Ahhhh - Javascript. The language that keeps surprising.

Never seen the

fn`txt`

notation before, but after a bit of googleling I realised that you want to use alasql as a tagFunction for a template string

I like the idea, even if im not sure its smart that its meant to return a string.

But...

Currently alasql is sync, and one way you can make it async is by providing an array as the first parameter

alasql(['select 1 as A', 'select 2 as B'])
=> 
[
  [
    {
      "A": 1
    }
  ],
  [
    {
      "B": 2
    }
  ]
]

I cant see how I can seperate the two situations (one using it as tagFunction and the other providing an array as parameter)- so for now I will reject the PR. If you have a good idea let me know and we can talk about how we do it.

@mathiasrw mathiasrw closed this Oct 6, 2022
@mathiasrw
Copy link
Member

Actually, I just saw that tagFunctions are called with a .raw parameter on the first argument. So we can seperate them.

image

But now I also understand that we will have to merge any merge fields (like ${name}) and I am not sure its smart to offer this kind of flexebility as it could make people not use ? notation to pass arguments to the SQL.

Hmm. Still thinking about this - but reopening...

@mathiasrw mathiasrw reopened this Oct 6, 2022
@lucioreyli
Copy link
Author

lucioreyli commented Oct 6, 2022

Let's step by step!

The first is identify if the function has called by alasql("query here") or alasql`queryhere` .
We can do this with the .raw property (thank you for this). Apparently only the template string functions have this property
image

Next step is format the query with values. I created a function to map every string to format (concat) with the corresponding parameter (value), like const userAuth = `user_id: ${user_id_variable}`
Screen Shot 2022-10-06 at 10 18 43 (2)

Example 2 breaking all keywords
Screen Shot 2022-10-06 at 10 22 52 2

And we have finally the parser.
Do you se it's as valid?

@mathiasrw mathiasrw force-pushed the develop branch 6 times, most recently from d813fd3 to be6cca0 Compare October 8, 2022 16:11
@mathiasrw
Copy link
Member

Still thinking about how best to use this interesting feature.

@lucioreyli lucioreyli marked this pull request as draft December 15, 2022 17:14
src/10start.js Show resolved Hide resolved
src/10start.js Show resolved Hide resolved
test/_test847.js Outdated Show resolved Hide resolved
@jimmywarting
Copy link
Contributor

jimmywarting commented Dec 21, 2022

i think this would be a cool addition to alasql

I'm just imagining being able to do things like:

sql`SELECT * FROM ${table} ORDER BY ${fieldName} ${direction}`
sql`SELECT author FROM books WHERE name = ${book} AND author = ${author}`

@mathiasrw
Copy link
Member

mathiasrw commented Jun 10, 2023

I really enjoy the convenience of this approach as well. However, we should be explicit about its usage. Consider this example:

let orderBy = `ORDER BY x desc, y asc`

alasql`
SELECT author 
FROM ${items} 
WHERE 
AND type = ${type}_v2
AND name = ${item} 
${orderBy}
`
  • If items is a string, it can be directly interpolated into the SQL template string or passed as a parameter. If it's an array, it should be passed as a parameter and represented as a ? in the query.

  • Since type is juxtaposed with another string, it must be directly interpolated into the query.

  • If item is a numeric value, it can be directly interpolated. If it's a string, it can be interpolated after being escaped. Alternatively, it can be passed as a parameter, which offers enhanced performance by allowing the SQL to be cached, independent of the name value.

  • orderBy should always be directly interpolated into the SQL as we can't replace it with a ?.

It seems we have a pattern where values surrounded by whitespace are to be converted into parameters. However, there are exceptions like orderBy, where users want to build the query, not just provide data. If it's intertwined with other text, we should always interpolate the string. For values with trailing or surrounding spaces, we have two options:

  • A) By default, convert it into a parameter, but provide a way to indicate when it should be directly interpolated.
  • B) By default, directly interpolate it, but provide a way to indicate when it should be a parameter.

If we choose A, I propose we use a : in front of the parameter to indicate that it should be a parameter:

let orderBy = `ORDER BY x desc, y asc`

alasql`
SELECT author 
FROM :${items} 
WHERE 
AND type = ${type}_v2
AND name = :${item} 
${orderBy}
`

Alternatively, if we opt for B, we could introduce a ~ in front of the field to indicate "inline"

let orderBy = `ORDER BY x desc, y asc`

alasql`
SELECT author 
FROM ${items} 
WHERE 
AND type = ${type}_v2
AND name = ${item} 
~${orderBy}
`

I have a preference for option B as it defaults to performance and things will break during development until people discuver why (although it might clash with the intuitive use of template strings).

Thoughts? Suggestions?

@mathiasrw mathiasrw changed the base branch from develop to v5 June 12, 2023 15:18
@mathiasrw
Copy link
Member

I made the logic for converting scenario B. Its currently only in a testfile.

https://github.com/AlaSQL/alasql/pull/1512/files#diff-77f7d769dccbde8555f2f34c49954f5bad1a7b19a4a615466452d25b87ac04cf

Im still not convinced that its smart to default to parameters.

Also: wondering if we should throw if people seek to inline objects or arrays. Is there any use for this at all?

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.

3 participants