Add with_columns/2 to assign column names to fragment sources

[greg-rychlewski]

In the doc for this function is the rationale for the feature.

I chose to create with_columns/2 instead of adding something like a columns option to from/join because I think the main use for this will be inside custom macros and for that it's nicer if it's attached directly to the source. It's also a simpler change.

Ecto SQL companion: elixir-ecto/ecto_sql#743

[greg-rychlewski]

I had talked about it in discord but the reason why {fragment, Schema} doesn't work here is because the user is hand rolling the query string and doesn't have access to the way that Ecto will order the Schema columns. And the order they write the columns in the string must match the column order definition in the generated query. For example if they wrote unnest(a, b) in the fragment string but the table definition ends up being f0 (b, a).

You could have some guarantees of some kind or ways to check but that is a pretty deadly footgun if someone changes the schema. I don't think that makes the {fragment, Schema} construct useless though because you can still have associations and stuff like that

[josevalim]

Thank you @greg-rychlewski! I think the combination of with_columns with fragment is a bit too subtle, as we are clearly extending fragment? I wonder if we could use splice_identifiers as part of a fragment instead?

  defmacro unnest(data, columns) do
    fragment("unnest(?) AS ?", splice(unquote(data)), splice_identifiers(unquote(columns))
  end

PS: we may have discussed this already, apologies.

[greg-rychlewski]

Oh yes I think we could definitely do something like that.

I was a bit unsure about it because it would be the only fragment helper that does not have a corresponding ? In the fragment string. Because we need to append it to the table identifier in the adapter (e.g f0 (col1, col2)).

I am definitely not against it though because every solution has some kind of drawback. Let me know if I’m misunderstanding what you said though.

[greg-rychlewski]

Oh sorry I did misunderstand. Your example had it in the fragment string. So if you do it that way then we will get double “as” when the adapter creates the sql. And it could get tricky figuring out if the string has a table identifier/dealing with conflicts in the other autogenerated table names

I’m still not against trying it out just letting you know why I opted out of that way

[josevalim]

So maybe we introduce it as syntax on from/join? It is a pity we already use :as though :(

[greg-rychlewski]

Yeah that was the second main option I was considering seriously. We could call it column_names or something like that. The main thing I didn’t like is that you could not associate the columns right on the source like we do for values. And also I guess we don’t want to allow it outside fragments because the rest of the sources have their names handled automatically.

But yeah that one and the one I submitted were the main 2 options I thought had the least drawbacks. I’m ok going with whichever one you like best

[greg-rychlewski]

One other random thing that popped into my head is introducing something like fragment_source where the second argument has to be the list of columns. I’m not sure it is a good api extension but just thinking of things that could be less subtle and still targeted to fragments

[josevalim]

We could support as: foo(column1, column2) but you are right that they are far apart. What about fragment(..., names: ...) or columns: ...?

[greg-rychlewski]

Yeah I think that would be simple! I’ll push it here so we can see how it looks

[greg-rychlewski]

@josevalim WDYT?

[josevalim]

Now I realize the splice_identifier would work like this:

from(f in fragment("select generate_series(?::integer, ?::integer) AS ?", ^0, ^10, splice_identifiers([:x])))

Although I don't know if database performance is worse.

[josevalim]

I have dropped some suggestions but I also realized there is another way to make it work with splice_identifiers. I have a slight preference for splice_identifiers but I like the current approach if wrapping it in a select has worse perforamnce.

[greg-rychlewski]

I think we can't guarantee there won't be worse performance across all databases and fragments, but I think it is unlikely to happen. The only thing I could potentially see as an issue is if the subquery was materialized for some reason. But I wasn't able to get that to happen in my own tests or find any examples when searching.

I could do splice_identifiers but I'll just give you a bit of a spiel why the current subquery approach has been bugging me for a while:

• It's a pretty unnatural way to write the query. Even though I wrote the original implementation I still sometimes get tripped up if I don't immediately remember why it has to be that way. For example you might go through all these iterations:

  • write "unnest(...)" and get an error
  • write "unnest(...) as .." and get an error because you don't remember/realize ecto is already creating the table name
  • finally write "select * from unnest(...) as ..." but even then it's kind of confusing because it's silently wrapped in a subquery and fragments are usually "as is"

• The query that comes out in the log can be pretty ugly to read especially if you have a few joins

[greg-rychlewski]

Now that I think about it there is actually a functional advantage to this way as well. Currently you cannot select the entire source if it is a fragment because we say "what are you talking about, you're a random string i don't know your columns". Having it in the metadata like this we could support it.

[josevalim]

Sounds good to me!

One last thing, after you mentioned subqueries, to make sure we cover all bases, why not:

map(fragment("unnest"), [:foo, :bar])

I am not quite sure I like it myself but it may suit better (or not) the subquery machinery!

[greg-rychlewski]

Oh sorry I'm not too sure what you were saying sounds good. Keeping columns: or changing to splice_identifiers?

[josevalim]

I meant the current PR, "Keeping columns", sounds good! Unless you think the map approach is better but I am not convinced by it either.

[greg-rychlewski]

Oh I get what you meant now. It’s a bit less intuitive to me but I can understand the idea

[greg-rychlewski]

I committed your review suggestions btw. So all ready to merge if you’re ok with it

[josevalim]

Ship it! ❤️

Currently you cannot select the entire source if it is a fragment because we say "what are you talking about, you're a random string i don't know your columns". Having it in the metadata like this we could support it.

Let's give it a try in a subsequent PR to validate this is really the direction we want to go?

[greg-rychlewski]

Yeah definitely :) I should be able today or tomorrow