Code review comment for lp:~lifeless/storm/with

Revision history for this message
Gustavo Niemeyer (niemeyer) wrote :

> I don't mean to whinge, but AFAICT I haven't had any guidance : no
> code pointers, design or structure pointers, other than 'something

There is a lot of information above which you've consistently
ignored. If you need details, ask for them please, and we'll
be very happy to assist.

> like ClassAlias'. As a specific example; you don't like 'with_', but I
> can't see any way to say to the compiler that a custom type should
> compile to the front of the query - before the
> SELECT/UPDATE/INSERT/DELETE.

James already answered that above:

"""
The WithTable object would compile similar to a normal table, but
note that it had been used in the state object. The compile
function for Select() could then generate the WITH clause if any
such tables have been referenced by the query.
"""

Note that tables are *already* compiled out of order in the query, so
it would be something along those lines:

1) Add with_tables to the State type, as James said
2) Make With() statements *return* the table name only (what is
   seen before a reference in a query, like "foo" so that
   "foo.column" works) during compilation
3) Make With() statements inject the WITH fragment within with_tables
   of the state during compilation
4) Make Select push and pop with_tables (see how auto_tables is handled
   there), and before popping with_tables, generate a WITH prefix in
   case it's non-empty, as James said.

> That could be very helpful - thanks. I'm timeshifted towards UK at the
> moment while we work through a backlog of RT tickets impacting the LP

I'm in Brazil, so -3 ATM.. feel free to book a time which is suitable for
you and within my awaken time (fine to be off hours).

> I've pushed up a sketch that may be what you meant, and toggled this
> back to needs review.

Ok, this is still not going in the right direction, but the above hints
might help you. Let me know if you have more questions.

review: Needs Fixing

« Back to merge proposal