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