Joinable subselects

[nextghost]

This changeset implements subselect objects that can be used in a join like a virtual table or passed to Expression.belongs(). Subselect objects can be created using Set.nested_select() which accepts the same arguments as regular select(). One additional argument, correlated, determines whether the subselect may reference tables from parent select (defaults to True which means referencing is permitted). Example usage:

total = table1.field2.sum().with_alias('total')
sub1 = db(table1.field2 != None).nested_select(table1.field1, total, groupby=table1.field1)
sub1 = sub1.with_alias('tmp')
result1 = db(table2.field1 == sub1.field1).select(table2.ALL, sub1.total)

total = table1.field2.sum().with_alias('total')
sub2 = db(table1.field2 != None).nested_select(total, groupby=table1.field1)
result2 = db(table2.field2.belongs(sub2)).select(table2.ALL)

---

I'm opening this pull request early for design discussion. Unit tests for Python 2.7, 3.4 & 3.5 and SQLite, MySQL and PostgreSQL are passing on my machine but there's still work to do. Comments and suggestions are appreciated.

Design changes

• Adapters and dialects must no longer resolve table names over and over through DAL.__getitem__(). They must pass Table/Select objects around.
• New methods for class Table:
  • query_name() - Full table name for query FROM clause, including possible alias.
  • query_alias() - The appropriate string that identifies the table throughout a query (e.g. for building fully qualified field names).
• Rows.__init__() now takes additional argument - list of Field/Expression objects corresponding to columns
• BaseAdapter._select() now returns a Select object instead of string
• BaseAdapter.tables() now returns a dict mapping table names to table objects instead of name list.
• BaseAdapter.common_filter() now accepts table objects as well as table names.
• many other BaseAdapter internal methods now take or return table objects instead of table names.

[nextghost]

Possible bug: ijoin_tables is never used.

[niphlod]

boys I so don't like _select() not returning a string. People may have been grow fond of that.

On another entire matter, I don't feel writing that much of code over a raw t-sql when in need of such complications...the proposed piece of pydal code to write is maybe longer than the query itself, and it's not readable at all.
Also, writing adapters for all backends could be a nightmare. I feel that a relevant part of the problems you can solve with correlated subselects would be better served by window functions.

[gi0baro]

@niphlod on the other hand I find the idea quite interesting, but I'm thinking about it in the matter of how pydal builds sql strings.
I think would be quite powerful to change the way the dialects builds sql strings, 'cause using custom classes and objects (that could implement the __str__ method and behave like strings with __repr__ so users wouldn't notice the difference) would allow us to really customize the sql generation. Moreover this opens the chance to implement caching on sql generation, and to simplify the flow of quoting-unquoting strings when talking to the driver (= performance improvements).

I think we should really move the discussion on a different "floor" here, but is definitely another story from this PR and the idea behind it. @mdipierro what do you think?

[mdipierro]

_select MUST return a string. it is in the specs.

select() makes a query and _select() gives me the corresponding SQL. Same for update and _update, insert and _insert, count and _count.

That is also needed for subselects like db(db.table.field.belongs(db(subquery)._select(db.table.field)).select()

We should not change it. It is ok to return an object that extends str.

[nextghost]

If I understand correctly how Python strings work, making the Select class an extension of str would require building the full SQL subquery string immediately in Select.__new__() and never changing it afterwards. I don't think that's a good idea and preparing subselects that reference tables from parent query would be more complex than it needs to be. Expanding a Select object in the belongs() expression is already implemented in SQLDialect.belongs(). Only Mongo and Google adapters reimplement that and they don't seem to support nested selects there anyway. But if you insist on _select() returning a string, I'll just revert the one-line change there and move it to nested_select() instead.

[mdipierro]

Yes. I would like that better. No need to break the current functionality since we can have a nested_select that is more clear anyway.

[codecov-io]

Current coverage is 66.38% (diff: 85.03%)

Merging #408 into master will decrease coverage by 0.12%

@@             master       #408   diff @@
==========================================
  Files            69         69          
  Lines          8908       9102   +194   
  Methods           0          0          
  Messages          0          0          
  Branches       1836       1987   +151   
==========================================
+ Hits           5924       6042   +118   
- Misses         2482       2548    +66   
- Partials        502        512    +10   

Powered by Codecov. Last update a77e208...883b97e

[nextghost]

It appears that nobody actually tested Rows.render(). Unit tests call it but don't check the output. If you called it as result.render(fields=[tab.f1, tab.f2, tab.f3]), those three fields would be rendered three times each. Fixed in the refactored version.

[gi0baro]

@mdipierro I'm for changing the behaviour: I think it's quite useless to stay on strings if we want to alter queries on the go, because it's just loosing time in building a string that will be rebuilt again and again. Having a SQLString object that behaves like a string, and that can be cached and takes care of quoting sounds quite promising regarding performance and customization.

@nextghost maybe before you continue with subselects we can design something more generic for the way strings are built in pyDAL, so that you can benefit from the new structure to implement subselects?

@niphlod opinions?

[nextghost]

GAE adapter should be working again, at least according to unit tests. IMAP, Mongo and CouchDB adapters don't seem to support joins anyway so I'll skip refactoring them. However, I highly recommend refactoring the rest of self.db[tablename] anti-pattern later. It can break if you get too creative with table aliasing.

Also, BaseAdapter should probably have its own nested_select() method so that NoSQL adapters can throw NotOnNOSQLError from it.

@gi0baro I'm almost done with adapter refactoring so there's no point in waiting for another big redesign. Just 2 or 3 more commits and I can wrap it up by writing unit tests. But here are some tips you might find useful:

• I recommend using different subclasses of Expression for each operation rather than storing a dialect method in the Expression object. E.g. the AndExpression should call Dialect._and() directly. Then you can easily test what kind of expression you're looking at using isinstance().
• The new interface should have preliminary support for using placeholders in the finished query. For example the query expansion method should return a tuple ('SQL string', [placeholder values]). If you look at Select.query_name() in this PR, it already works like that.
• The return value of Table.on() should not be an instance of Expression. You should probably keep it only for backward compatibility and instead implement Join objects that allow arbitrary tree of joins. The current interface for example doesn't allow you to do this: SELECT ... FROM a LEFT JOIN (b INNER JOIN c ON ...) ON ....

[niphlod]

@gi0baro : on top of the notes on #411, I don't think that caching the generated query will solve THAT amount of performance to be noticeable.
Finalizing on the "_select() returning a string", I'd say that if
q = db(db.table.id>0)._select()
db.executesql(q[:-1])
works with what you want to make _select() do, we should be fine.

If we want to really save time, it should be saved in avoiding expanding arguments by ourselves, as the 99% of the drivers are mature enough to do it by themselves (and query plans for prepared statements are cached on the backend, that's one real speedup). It'll eventually close also the requirement for using binaries instead of base64 encoding them (and all the memory issues in heavy-processing pieces of code).

[nextghost]

@niphlod Sorry, no. The bug is very much still present here. This pull request is already huge and I will not add another 200 lines of code just to fix something I didn't break myself.

We can discuss the solution in issue #411. I'll open a new pull request once this gets merged.

[mdipierro]

what's the status here?

[niphlod]

@mdipierro : everything scheduled for the next or the following release (meaning hopefully for december)

[nextghost]

@mdipierro I still haven't had time to finish the unit tests. If things go well, I might get it done next week.

[nextghost]

Aaaand, done. If you don't have any further comments or suggestions for this pull request, you can merge it.

[gi0baro]

@nextghost @mdipierro will review this carefully in the next days.

[gi0baro]

I just made few questions and a suggestion for a change. I will also perform a performance test before the merge. Thank you for the hard work @nextghost

[gi0baro]

@nextghost what about this? Should be fixed?

[nextghost]

Fixing it would break backward compatibility with third-party code. outer_scoped is currently a list of internal table names. It would have to be changed into a dict of table objects, otherwise there is no way to check for conflicts (same name referencing different objects).

[gi0baro]

@nextghost can you explain this better? Which is the correct syntax?

[nextghost]

join_tables gets expanded below the comment into something like LEFT JOIN tab1, tab2, tab3, .... MySQL and PostgreSQL both raise syntax error on that but SQLite seems to accept it. At the very least, the expansion should be handled by the dialect, not SQLAdapter._select_wcols().

The ON clause is mandatory for each table in LEFT JOIN in MySQL and PostgreSQL.

[gi0baro]

@nextghost ok, will inspect that apart from this PR

[gi0baro]

@nextghost same as the other comment

[gi0baro]

@nextghost you should use dialect's _as method instead of checking the adapter type

[nextghost]

This bit was copied from Table.__str__(). Should I fix both?

[gi0baro]

@nextghost would be awesome :)

[gi0baro]

@nextghost can you also please update the description of this PR, also adding a quick example of the usage?

[nextghost]

@gi0baro I've fixed table/subselect alias expansion as you requested and added another minor bugfix. I've also updated PR description and added usage example. (Should I remove the old description as well?)

But before you merge it, you should also take another look at the two pieces of code which I've highlighted right below the PR description.

[gi0baro]

Thank you @nextghost! The description looks good to me right now.
I will update this later during the week regarding your two comments and some benchmarks.
I feel quite sorry about the slowness of this but it's quite a large PR you know? :)

[gi0baro]

@nextghost ok, the performance seems quite the same from my benchmarks.
Regarding your comments, I would like to open up two new issues for those (as for the wrong syntax about the left joins) and discuss about them separately from this.

If you don't have any other considerations, I will merge this.

[nextghost]

I have nothing more to add, you can merge this.