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

Add splice to fragment #4228

Merged
merged 15 commits into from
Jul 14, 2023

Conversation

greg-rychlewski
Copy link
Member

@greg-rychlewski greg-rychlewski commented Jul 13, 2023

This is a proposal to add a splice capability to fragment. It is taking inspiration from literal/1 as well as unquote_splicing.

The idea is that the user would supply a list argument to fragment and then this would be spliced into the query string. Something like this

a = 2
b = 3
query = from p in "posts", where: p.id in fragment("(?, ?, ?)", 1, splice(^[a, b, 4]), 5)

IO.inspect query
#Ecto.Query<from p0 in "posts", where: p0.id in fragment("(?, ?, ?, ?, ?)", 1, 2, 3, 4, 5)>

There are several use cases where this could come in handy:

  1. Wanting to change the default Ecto behaviour using fragment when you require a variable number of arguments. For example changing the where operator behaviour: Switch to using = ANY(SELECT unnest(?)) instead of = ANY(?) for IN operator #2570 (comment)
  2. Wanting to use VALUE lists that might have different sizes.
  3. Some databases allow variadic functions. This would allow users to supply a variable number of arguments using the same fragment. This is a good fit because fragments would need to be used to call these types of functions anyway.

If the proposal is accepted I could finish up the PR. Just didn't want to get ahead of myself before receiving feedback.

@greg-rychlewski
Copy link
Member Author

greg-rychlewski commented Jul 13, 2023

I just realized they are not being collected as parameters. I could see if that could be handled in the planner but not sure about MySQL because it uses ? instead of ?n

edit: Now I'm really doubting this feature is viable. If we want to collect the parameters in the planner then we need to traverse all the exprs before hitting the cache. And we don't want to do that. And I don't think this is a good idea without collecting them as parameters because then the # of cached queries could explode.

Probably I need to think if there is a completely other way to do something like this. Or maybe the query can just not be cached when using this. If that's a thing.

@greg-rychlewski
Copy link
Member Author

greg-rychlewski commented Jul 13, 2023

I think something like this could work actually.

edit: no actually it can't. sorry for the noise.

@josevalim
Copy link
Member

Would it make a difference if we use the notation inside the fragment? Something like "(?, *?, ?)"?

@greg-rychlewski
Copy link
Member Author

greg-rychlewski commented Jul 13, 2023

I believe it will be the same issue. It looks to me like the underlying issue is being able to convert a runtime list from a single param to a dynamic number of params.

WTDY of something like this? I am trying to "flag" the parameters so that they can be treated properly at runtime if needed. Probably this is not a good flag but was just curious what you thought of the general idea?

P.S If this is ok then actually I like your inline syntax better. I think the same strategy can be done for that. Maybe need to think a bit if people would commonly use * in their fragment and would now have to escape it.

@josevalim
Copy link
Member

Honestly, it has been a while since I played with this, so I cannot quite recall. What i remember is that each list length becomes a different cached query. You can look at how MyXQL handles this, because they support variadic in.

@greg-rychlewski
Copy link
Member Author

Oh I see it now. Thank you for pointing me to that.

@greg-rychlewski
Copy link
Member Author

WDYT about this way? If it's good I can add the docs.

Companion ecto sql PR: elixir-ecto/ecto_sql#535

@josevalim
Copy link
Member

LGTM! Please don't forget to add splice to Ecto.Query.API as well. :)

@greg-rychlewski greg-rychlewski merged commit eb03f45 into elixir-ecto:master Jul 14, 2023
7 checks passed
@greg-rychlewski greg-rychlewski deleted the splice_fragment branch July 15, 2023 01:11
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.

2 participants