Horizon: Prevent .order from being called without .limit or .above/.below

[samuba]

Version: 1.0.3

This is pretty essential in my opinion and I was very surprised to see this not working.

This will always return unordered lists: chat.order("id").watch()
It does properly work with fetch() tough. This works as expected: chat.order("id").fetch()

Here is some sample output:

chat.order("id").watch().subscribe(x => console.log(JSON.stringify(x)))
[
    {
        "$hz_v$": 0,
        "from": "me",
        "id": "0875b9c2-08c4-4bc9-a9fd-c2598dfb840e",
        "text": "first",
        "timestamp": "2016-05-29T07:19:52.627Z"
    },
    {
        "$hz_v$": 0,
        "from": "me",
        "id": "024264b2-a3f8-4c08-b152-aaad47211272",
        "text": "third",
        "timestamp": "2016-05-29T07:19:57.626Z"
    },
    {
        "$hz_v$": 0,
        "from": "me",
        "id": "fd83ba30-488f-400d-93db-0c2b9a9f5d16",
        "text": "sixtht",
        "timestamp": "2016-05-29T07:20:33.598Z"
    },
    {
        "$hz_v$": 0,
        "from": "me",
        "id": "6ca789d2-4cba-4871-8325-ff2b2077efba",
        "text": "fifth",
        "timestamp": "2016-05-29T07:20:27.001Z"
    },
    {
        "$hz_v$": 0,
        "from": "me",
        "id": "1be1b4cb-26f0-4dbc-8e6d-2db8e987a3c4",
        "text": "second",
        "timestamp": "2016-05-29T07:19:55.538Z"
    },
    {
        "$hz_v$": 0,
        "from": "me",
        "id": "994556cd-0d2e-4a2e-a525-b12eb91df443",
        "text": "fourth",
        "timestamp": "2016-05-29T07:20:00.546Z"
    }
]
​
​
chat.order("id").fetch().subscribe(x => console.log(JSON.stringify(x)))
[
    {
        "from": "me",
        "id": "024264b2-a3f8-4c08-b152-aaad47211272",
        "text": "third",
        "timestamp": "2016-05-29T07:19:57.626Z"
    },
    {
        "from": "me",
        "id": "0875b9c2-08c4-4bc9-a9fd-c2598dfb840e",
        "text": "first",
        "timestamp": "2016-05-29T07:19:52.627Z"
    },
    {
        "from": "me",
        "id": "1be1b4cb-26f0-4dbc-8e6d-2db8e987a3c4",
        "text": "second",
        "timestamp": "2016-05-29T07:19:55.538Z"
    },
    {
        "from": "me",
        "id": "6ca789d2-4cba-4871-8325-ff2b2077efba",
        "text": "fifth",
        "timestamp": "2016-05-29T07:20:27.001Z"
    },
    {
        "from": "me",
        "id": "994556cd-0d2e-4a2e-a525-b12eb91df443",
        "text": "fourth",
        "timestamp": "2016-05-29T07:20:00.546Z"
    },
    {
        "from": "me",
        "id": "fd83ba30-488f-400d-93db-0c2b9a9f5d16",
        "text": "sixtht",
        "timestamp": "2016-05-29T07:20:33.598Z"
    }
]

[danielmewes]

Thanks for the bug report @samuba.

I bet this is because RethinkDB doesn't support changefeeds on ordered streams without a limit. We need to think of a good way for handling this.

One option might be implicitly adding some relatively high limit (e.g. 1,000) in the Horizon server, and erroring if the limit gets exhausted (the error message could suggest specifying a larger limit manually).
Another option is to have the RethinkDB server do basically the same thing, subject to the array size limit or something similar.
As a third option, we could disallow ordered watch queries without a limit in Horizon in general.

[mlucy]

Inside of RethinkDB, ordered changefeeds without a limit don't really mean much because the changes will still be sent to the client in the order they happen. Inside of Horizon, it might make sense to just subscribe to all changes and maintain an ordered list client-side.

[danielmewes]

Well, the includeOffsets option for limited ordered changefeeds currently makes it unnecessary for the Horizon server to implement ordering I think (@Tryneus or did you still have to implement ReQL datum comparison?).

It seems conceptually slightly better to me if the RethinkDB server takes care of maintaining the ordering, rather than having the Horizon server do it. However I think that includeOffsets currently has O(n) complexity for every change where n is the current size of the result set. So we would either have to implement this more efficiently, or limit the number of allowed results on such a changefeed.

[mlucy]

@danielmewes -- I was imagining that neither RethinkDB nor the Horizon server would implement it, and that the Horizon client would just take care of splicing the changes into its local ordered data structure. That way we don't keep an extra copy of the data hanging around on the server.

[deontologician]

I think the reason RethinkDB doesn't do this is a valid reason to not allow users to do this in Horizon. If you don't add a limit, you're much more likely to be accidentally shooting yourself in the foot. Much better to just require them to specify a limit.

[deontologician]

We originally did not allow .order without a .limit on collections, and everybody kept saying it was needed for convenience so we added it.

[danielmewes]

@mlucy Ah got it. That would make sense.

I think having it in a non-working way is bad.
How about we keep allowing it for fetch requests (where it works), and throw an error when trying to subscribe to it with an error message that suggests adding a limit?
Does that sounds reasonable to everyone?

[mlucy]

@deontologician -- RethinkDB doesn't do this because it doesn't mean anything useful in RethinKDB, so the only reason people try to do it is if they have a misconception about how it works. table.changes() and table.orderBy(...).changes() are identical; they both send you all changes as soon as they occur. (include_offsets is the exception to this, but it didn't used to exist and isn't generally a good idea to use anyway.)

What's different about Horizon is that it's constructing an ordered data structure for people in the client, so there's actually a useful semantic difference between collection.watch and collection.order.watch -- they both receive all the changes as soon as they occur, but they apply them to the local data structure differently.

[deontologician]

I could go with that, though the fetch will fail in the same circumstances the watch will. At some point, not having a limit will blow up huge and you'll crash the browser tab

[danielmewes]

@deontologician I don't think fetch will fail. The reason changefeeds don't have the order is because of how initial results are implemented. With a normal fetch, the results should be arriving in order.

[deontologician]

@mlucy I was just referring to the case where include_offsets is activated, where the lack of a limit means a huge blowup in memory usage.

[mlucy]

@danielmewes -- I think long-term we'd want .order.watch to work and be implemented entirely client-side. If that's hard to do, I think in the interim erroring and telling people they need to set a limit is a good stopgap.

[deontologician]

@danielmewes The fetch will fail when you get 10Ks or 100K's of documents in your collection and don't specify a limit. We don't have the concept of a cursor, so it's all fetched before anything is returned.

[deontologician]

The reason I think it's bad to provide .order.watch without requiring a limit is because it's one of those queries that seems to work fine in development with a few documents in your collection, and then later gradually makes your page slower and laggier under production loads. If someone wants to do something like .order.watch, we should push them towards the pagination api (admittedly not created yet)

[danielmewes]

@deontologician Oh I see. Does that accumulation happen on the Horizon server or in the Horizon client? Are we actually enforcing a limit on the result size in Horizon currently?
Also this is not specific to order right?

[deontologician]

Right, this reasoning also applies to stuff like collection.watch() etc. The accumulation happens in the client. The server I think sends batches, but we wait for all batches to come in before emitting results.

[danielmewes]

I think accumulation in the client is fine, since it's not an issue for scaling Horizon to more users. (plus there's normally no way around it, since the browser has to render the results somehow)

[deontologician]

Renamed this issue to reflect what the thread has become about

[deontologician]

Ok, so this is resolved in 2.0.0, we basically just set an implicit limit if you give an order but not a limit. Eventually we could implement sorting on the client, but a lot more bookkeeping would be needed