Offset and Limit not present in BaseQuery.String/ToSql

[film42]

I was writing some code that conditionally applies a LIMIT and OFFSET; and while testing ended up very confused because calling query.ToSring() or query.ToSql() did not apply my LIMIT and OFFSET. After grepping through the kallax code I realized that these aren't applied until the Store.Find method is executed:

https://github.com/src-d/go-kallax/blob/6e319000292790068f9b6efaa05c38725d507b56/store.go#L385-L392

I was just wondering if these are supposed to be applied here, or if this should remain a concern of BaseQuery.

Thanks!

[nadiamoe]

I'm not 100% percent sure on this, but I think these is not done on BaseQuery because of how squirrel StatementBuilders work. As they are meant to be as agnostic as possible and offset and limit do not apply to all types of queries, the base interface does not contain these options. BaseQuery works on top of this interface, so it can't add those options itself.

There are a number of issues concerning the squirrel integration, and the original plan was to drop the dependency and make Kallax generate its own statements. Unfortunatly, currently neither I nor the original developers have enough time to get this task done. I'd suggest to patch your tests however you can, as this issue (and other squrrel-related ones) are probably going to stay with kallax for some time. I'm sorry for the inconvenience.

[film42]

@roobre I see how you might not want to apply limit and offset all the time, but that logic to selectively apply them (the same code referenced above) can live in the Query.compile method and still produce the same result, right?

Only edge case is if a different method compiles the query and does some other stuff prohibiting a limit and offset from ever being applied. Though I haven't looked very hard for this edge case, I think it's probably something we can work around. Would it be OK if I open a PR attempting to refactor this?

[nadiamoe]

Last time I checked it was an issue with the interfaces squirrel expects for some methods. Of course you can give it a try, and I'll be happy to accept it if it doesn't need a big refactor (like changing unrelated methods due to returning very different things, or changing too much the program flow) and of course it doesn't break any functionality of the exposed APIs.