ocsigen
Add the array_agg aggregator
You should not use row_t here as it is mean to denote not a single SQL value, but a whole row of a SQL selection. I think < t : 't; blabla > group -> < t : 't array_t; nul : nullable > would be the expected type for array_avg.
That said, I feel a bit reluctant to add partial support for arrays in this way. If we want to make a jump to support arrays, we should probably provide more stuff (such as array access and construction). I would rather support string_agg as a first step, which is very simple to add from a typing perspective and raises no delicate questions whatsoever, and think a bit more about how arrays should be supported -- or wait for a more complete pull request.
I've rebased my PR with only string_array_agg (+ the string array type)
Do you think it's a good solution ? (see the last commit)
EDIT: Rebased because of a typo in the commit message
EDIT2: Oups, sorry. There is a bug. I'll make a fix right a way.
I am still not convinced that this is the right solution. In particular, I doubt this approach could be extended to handle "really polymorphic" arrays. I may try to experiment with other things (e.g. the idea of separating the caml-type from the sql-type of a sql value), but this requires design work and you should not expect arrays to be upstream soon.
On the other hand, I would have nothing against other aggregate functions such as string_agg and xmlagg. string_agg is only supported by recent PostgreSQL versions, but it can be encoded in older versions with array_agg (we can use array_agg internally when producing queries, the problem is with exposing it directly in Macaque); there is nothing in place to configure Macaque's query rendering based on the available SQL version, but this can be added in the future if asked.
My advice would be to try using one of these simpler aggregators for your workflow. I know it's not as nice conceptually, but elegance requires more work.
Is there a change for this PR to be merged ? Otherwise I've to backport c43b07b to be compatible with pgocaml 2.0.
What I think Macaque would need to properly support arrays is to move from a type 'a Sql.t, where the single parameter 'a encodes both the SQL type of the value and (as a sub-parameter) the OCaml type of the value, to having two different parameters ('a, 'b) Sql.t where 'a corresponds to the SQL-side type, and 'b to the OCaml-side type -- or conversely.
As long as we only had non-parametric type, we didn't need this distinction. However, representing the SQL type of arrays of < typ : int > Sql.t as < typ : int > Sql.array_t Sql.t is wrong, as it gives no way to fetch the type int array (or int list) of the corresponding OCaml value. What we would need is to move from (Sql.int_t, int) Sql.t from (Sql.int_t Sql.array_t, int array) Sql.t.
Unfortunately, that is an invasive change that does require some work, and I obviously haven't have time to work on this on the past few months. So there is a choice to make, which is:
- delay this feature in the upstream Macaque branch until someone (me, you, or whoever is interested) gets time to implement it
- integrate your proposed patch, knowing that it is not the proper solution, but it gives access to a feature that you apparently desire
In the second case, I would need to understand the upgrade path from there. What happens, if we finally implement proper polymorphic arrays, to the old code using the old interface? In any case, I suspect that doing the change might break compatibility (it may be possible to do that in a compatible way, though), so maybe expecting users having to change their array-manipulating code is also fine.
I don't understand. It's already possible to fetch the type int list from Sql.array_t as it is defined as:
class type ['t] array_t = object
constraint 't = < typ : 'ty; arrayable : unit; .. >
inherit ['ty option list] type_info
end
So we just have to decompose this object into < typ : 't; .. > with 't being our int list.
I've rebased and added a commit which simplifies/cleans a bit the types.
Hm, it's my turn to be confused: if the proposed array_t is expressive enough to express polymorphic arrays, then why did you include all those ugly foo_array_t type instead of letting people write foo_t array_t directly?
Removed. Simply because I worked on this by several separated steps and int32_array_t already existed.
Now, there is one thing which can be interesting to do: Remove this ugly option type (you expressed that idea once).