CLN: Removing Query class

[datapythonista]

The Query class and subclasses have a tricky implementation, and are causing problems in #2777.

The class SQLClient instantiates the Query class, passing itself as a parameter, and then calls it's execute method, which will call the client _execute method. This seems nonsense, and just implementing the functionality of Query (which is quite simple) in SQLClient seems to be much more reasonable.

[datapythonista]

@jreback this is finally passing the tests, if you don't mind having a look.

[jreback]

yeah looks good. assume this has no effect on perf anywhere?

prob worth adding a release note (even though in theory users shouldn't be affected)

[datapythonista]

Added the release note. I don't see how this could affect performance (the code is exactly the same, just not using Query but methods of Client instead).

[icexelloss]

LGTM but I am little concerned about the implications of this change on other "Query" related class such as QueryBuilder, QueryPipeline etc.

Do those classes continue to work with the removal of the Query class? Or perhaps those are not used as well?

[icexelloss]

I am not familiar with the sql backends so maybe they are not related as all (only share the same word query in the class name)

[datapythonista]

LGTM but I am little concerned about the implications of this change on other "Query" related class such as QueryBuilder, QueryPipeline etc.

Do those classes continue to work with the removal of the Query class? Or perhaps those are not used as well?

QueryBuilder is the core class of SQL backends. It's about building the SQL, while Query was independent to it, and called directly from the Client, once a SQL string was available (whether because it was written manually or generated from an Ibis expression). QueryPipeline was an empty class, never used, that was a placeholder for a future idea, but never implemented, so it's removed here.

Not sure what's the concern regarding those classes, but to me this PR is a quite important milestone. This code was generating problems when trying to clean up things, and also was very confusing (for no reason), as explained in the description. Also, based on your comment, I think it helps also to not confuse things with the other unrelated concepts, since Query here was more executing processing a cursor and generating a pandas dataframe, and in other places query refers to building a SQL.

Just to note, this is not a final stage of things. This is important since Query was very tricky, and very poorly decoupled from the Client, since it was supposed to do things, that then it was expecting the client to have certain non-obvious methods that it was calling. Making it a nightmare of hidden shortcuts. After this is merged, all the much bigger mess of the QueryBuilder is fixed, and lots of duplicate code in the backends is removed, it'll be quite easy to discuss specific changes on how to structure things and make the changes. For now IMO this is just needed in order to be able to have a decent code that can be maintained.

[datapythonista]

@icexelloss let me know if my previous comment makes sense, or if you've got any other concern. Would be nice to merge this soon, as it's blocking me from the working in the other PRs I've got open. Thanks!

[icexelloss]

Sounds good to me! +1

[jreback]

thanks @datapythonista very nice

[emilyreff7]

@datapythonista removing the Query class breaks this usage in ibis-bigquery:

https://github.com/ibis-project/ibis-bigquery/blob/main/ibis_bigquery/client.py#L174

I haven't taken a detailed look at this refactor PR yet but do you see a way to refactor this usage as well?

[datapythonista]

@datapythonista removing the Query class breaks this usage in ibis-bigquery:

https://github.com/ibis-project/ibis-bigquery/blob/main/ibis_bigquery/client.py#L174

I haven't taken a detailed look at this refactor PR yet but do you see a way to refactor this usage as well?

Unfortunately there is no way to make these improvements without breaking the backends. It's not only bigquery, all them are affected. For the ones in this repo the PR updates them to the new way of implementing things. For the rest (bigquery, omnisci, mssql...) it'll need to be done before the next release. The changes required are the same as in this PR, if you check clickhouse for example, you'll see what needs to be done. It's quite small.