Created attachment 293555 [details] [review]
filter: Get the table from the type passed as argument

This really shouldn't change anything at all, as there would be no
reason to pass a type different from the one inside the private pspec
attribute of the GomFilter. (and this is an internal function, so we can
be sure of it).

However, this is necessary to implement the ORDER BY filters, which
won't have a single private pspec.

Created attachment 293556 [details] [review]
filter: Drop unused argument to the get_table function

Created attachment 293557 [details] [review]
filter: Support ordering queries

Created attachment 293564 [details] [review]
filter: Support ordering queries

Review of attachment 293555 [details] [review]:

"which won't have a single private pspec."

It won't have private pspecs at all, or it will have multiple?

I think this code might break if we have:
GomResource
     |___>  GenericResource
          |____> SpecialisedResource

If the tables are different for Generic and Specialised, there could be a problem. A test case would make sure that's not the case...

Review of attachment 293556 [details] [review]:

If we assert that the previous patch works, that looks fine.

Review of attachment 293564 [details] [review]:

Looks good!

::: gom/gom-filter.c
@@ +515,3 @@
       return ret;
+   case GOM_FILTER_ORDER_BY: {
+      GomOrderByParam *o;

You only use it inside the loop below, so declare it in the loop's body instead.

I don't think that putting sorting into a filter is the right move. In particular, Filters were meant to be a sort of AST for the query part of a statement. ORDER BY, is part of the sorting/aggregation modification part of the query. (For example, a smart index will walk the index backward if ORDER BY is DESC and the index is ASC).

I think we probably want a new type that represents sorting or aggregation modifications.

So, if we go with a different type, I guess what we could do is something like this:

  GomResourceGroup *gom_repository_find_sync (GomRepository  *repository,
                                              GType           resource_type,
                                              GomFilter      *filter,
                                              GomOrderer     *sorter, 
                                              GError        **error);

Do we modify the gom_repository_find_(a)sync API to add this new parameter?

Or do we add a new gom_repository_find_order_(a)sync?

Or something completely different? :)

I'm wondering if we should also add something for groupby+having, GomAggregation or something.

Is there anything else that would fit in with sorting? "Orderer" feels a bit strange of a word to me. Some other options might be:

 - Collation
 - Sorting

Another good question to ask ourselves is if this sorting should be a tree like filters are. Should they be an array/linked-list/chained?

Say you want to sort by 3 keys, first in ASC, second in DESC, and third ASC. What might that look like?

> Say you want to sort by 3 keys, first in ASC, second in DESC, and third ASC. What might that look like?

My patch (making ordering a filter) actually handled that. :)

The way it worked was:

  GomFilter *filter;
  GomFilter *sorted;

  filter = gom_filter_new_...
  sorted = gom_filter_new_order_by(filter,
                                   EPISODE_TYPE_RESOURCE,
                                   "season-number",
                                   GOM_FILTER_ORDER_DESCENDING,
                                   EPISODE_TYPE_RESOURCE,
                                   "episode-number",
                                   GOM_FILTER_ORDER_ASCENDING,
                                   NULL);

You'd just specify arguments by groups of 3:
- the resource type on which to orderthe query
- the column/property on which to order
- ascending or descending

And you could add as many of those groups as you wanted, to generate as complex an ORDER BY as you want.

The above example would generate the following query:

  SELECT * FROM 'episodes'
  WHERE ...
  ORDER BY 'episodes'.'season-number' DESC, 'episodes'.'episode-number'

So, I know we're saying we don't want to (ab)use GomFilter for the ORDER BY, but does the above fit with what you have in mind, if we make it a new GomOrderer class? (or any other name that doesn't sound like it was made up by a French :P)

Comment on attachment 293564 [details] [review]
filter: Support ordering queries

Marking as reviewed as discussion is still on-going.

Created attachment 306770 [details] [review]
Ignore some new files

Created attachment 306771 [details] [review]
sorting: Support ordering queries

Created attachment 306772 [details] [review]
sorting: Add an async method

Comment on attachment 293555 [details] [review]
filter: Get the table from the type passed as argument

Marking as obsolete since we decided to not (ab)use filters for ORDER BY.

Comment on attachment 293556 [details] [review]
filter: Drop unused argument to the get_table function

Marking as obsolete since we decided to not (ab)use filters for ORDER BY.

Comment on attachment 293564 [details] [review]
filter: Support ordering queries

Marking as obsolete since we decided to not (ab)use filters for ORDER BY.

As per the discussion here, I made a new type for sorting operations.

A few things I'm unsure of:

1. I have no idea how GROUP BY or HAVING should work. I haven't used either in a very long time with SQL (or other ORMs), and I have no need for either in Gom. As a result, I haven't done anything related to them in my latest patch series.

2. I'm adding a new set of gom_repository_find_sorted{,a}sync functions, to avoid breaking the API. However, it feels a bit clunky to be adding a new function that takes in the new GomSorting parameter, especially if we later on add something for GROUP BY, and we add one more function at that point. Maybe we should consolidate all this in a single gom_repository_find_{,a}sync function?

Review of attachment 306770 [details] [review]:

Sure.

Review of attachment 306771 [details] [review]:

Looks good otherwise.

::: gom/gom-repository.c
@@ +607,3 @@
+ */
+GomResourceGroup *
+gom_repository_find_sorted_sync (GomRepository  *repository,

That code looks fine, but gom_repository_find_sync() and gom_repository_find_async() should be a call to the find_sorted functions, with a NULL GomSorting, avoiding duplicated code.

Review of attachment 306772 [details] [review]:

::: gom/gom-repository.c
@@ +685,3 @@
 
+void
+gom_repository_find_sorted_async (GomRepository       *repository,

Ditto the other patch, this should help simplify gom_repository_find_async().

Attachment 306770 [details] pushed as 18964de - Ignore some new files

> That code looks fine, but gom_repository_find_sync() and gom_repository_find_async()
> should be a call to the find_sorted functions, with a NULL GomSorting, avoiding
> duplicated code.

Not only does it avoid duplicated code, it also makes the patches much simpler, and easier to review.

New patches are coming.

Created attachment 307043 [details] [review]
sorting: Support ordering queries

Created attachment 307044 [details] [review]
sorting: Add an async method

> 2. I'm adding a new set of gom_repository_find_sorted{,a}sync functions, to avoid
> breaking the API. However, it feels a bit clunky to be adding a new function that takes
> in the new GomSorting parameter, especially if we later on add something for
> GROUP BY, and we add one more function at that point. Maybe we should consolidate
> all this in a single gom_repository_find_{,a}sync function?

In the interest of keeping track of IRC conversations: Bastien said this was fine for now, and asked me to open a new bug report about whether it was worth eventually breaking the API, consolidating those functions in a single find{,_async} one at some point.

I'll do that if/when we decide to merge the patches for this bug which add the new gom_repository_find_sorted{,_async} functions.

Review of attachment 307043 [details] [review]:

Looks good.

Review of attachment 307044 [details] [review]:

Looks good.

Attachment 307043 [details] pushed as 423e1dc - sorting: Support ordering queries
Attachment 307044 [details] pushed as f6b9b84 - sorting: Add an async method