for _compiler_dispatch, it looks mostly OK - i think "isselect" isn't needed? Or if it's meant to accommodate a SELECT that's inside of an UPDATE or DELETE, I think you'd need to use a stack based approach so that when you exit visit_select() the "isselect" flag is reset. The isdelete/isupdate/isinsert flags are meant to be mutually exclusive. I'd just peek inside of self.stack, perhaps via the is_subquery() method, or something more specific, if you truly need to detect SELECT inside of UPDATE/DELETE for that. Looks OK otherwise.
"only()" remains more troubling. I can see the difficulty, it's somewhere in
between a "hint", and a directive that actually produces a different result,
which is unlike a hint. But using a new selectable construct adds a large
amount of complexity that I still see as unwarranted.
Creating a wrapper around a selectable requires that all the Column objects be
"proxied" into new ones. Column has a ".table" attribute that points to the
parent table - so you need to do more than just copy over table.columns to
self._columns - this breaks the Column which you can see if you run a regular
select against the table after the only():
from sqlalchemy import *
m = MetaData()
mytable = Table('mytable', m, Column('id', Integer, primary_key=True),
Column('data', String))
from sqlalchemy.dialects.postgresql import base as postgresql
mytable_only = postgresql.only(mytable)
print select([mytable_only.c.id,
mytable_only.c.data]).compile(dialect=postgresql.dialect())
print select([mytable.c.id,
mytable.c.data]).compile(dialect=postgresql.dialect())
output:
SELECT mytable.id, mytable.data
FROM ONLY mytable
SELECT mytable.id, mytable.data
FROM ONLY mytable
So at the very least, a construct like "only()" would usually subclass Alias so
that the column proxying is handled.
Also let's not forget that this feature needs to work with the ORM too. In
that case, I guess we'd need to use aliased(), something like:
only_of_class = aliased(MyClass, alias=only(MyClass.__table__))
Through all of this, only() as a separate construct still seems unwarranted
because as far as I can see, only() doesn't represent a new lexical identity
(which is the main job of a FROM element). That is, you wouldn't have
something like this:
SELECT * FROM ONLY mytable, mytable WHERE ...?
Assuming I'm understanding ONLY correctly, if you need the same table twice in
the statement, you still need to use alias(), so not much changes there from
how things work now (including when using with_hint(), which supports aliases).
When you say "ONLY mytable", that isn't creating a new lexical identity within
the statement. You're modifying an existing lexical identity, the identity
denoted by the Table or Alias. So even though not quite a hint, still acts a
much more like a hint than a new lexical identity.
The attached patch illustrates some small changes to compiler that lets ONLY
come right through as a table hint, without the need to change much else.
Usage is like:
select([mytable.c.id, mytable.c.data]).with_hint(mytable, "ONLY",
dialect_name="postgresql")
mytable_alias = mytable.alias()
select([mytable_alias.c.id, mytable_alias.c.data]).with_hint(mytable_alias,
"ONLY", dialect_name="postgresql")
mytable.delete().with_hint("ONLY", dialect_name="postgresql")
This approach, while stretching the definition of "hint" a bit, makes usage of
existing functionality and uses an API that is already familiar in other use
cases, with no dialect import required and no complex table proxying going on.
We end up making with_hint() a more capable system rather than adding an
entirely new and narrowly-scoped system elsewhere.
If you approve, we can complete and clean up this approach, write some tests,
fix up the isselect thing, and all three patches can find their way in.
On Jun 9, 2012, at 6:51 PM, Ryan Kelly wrote: > On Sat, May 05, 2012 at 08:00:20PM -0400, Michael Bayer wrote: >> >> On May 5, 2012, at 7:33 PM, Ryan Kelly wrote: >> >>> List: >>> >>> I am currently trying to add support for FROM ONLY to the postgresql >>> dialect. FROM ONLY is used when you wish to query a table which has other >>> tables that inherit from it, but you ONLY want results from the parent >>> table you are referencing. More information can be found here: >>> http://www.postgresql.org/docs/current/static/sql-select.html#SQL-FROM >> >> OK, this looks like a nice patch, though I'm not sure it's consistent with >> how we've approached this kind of SQL feature in other cases. "ONLY" looks >> a lot like an optimization hint added to the FROM clause. We already have an >> API for this, called with_hint(). with_hint() is intended to be supported >> by all of INSERT, UPDATE, DELETE in addition to SELECT. I'm not sure at >> the moment what it does with PG right now but it might be a more appropriate >> approach. Take a look and let me know if you have thoughts on that. > I had seen that when I started implementing this, but I felt that 'logically' > speaking, ONLY was more like an alias than a hint. I tried just now to > implement it using hints, but a lot of the code makes assumptions about > the location of hints with regards to the table name, i.e., the hint > always comes after the table name. ONLY always appears before. And I'm > not entirely sure how it would work if the same table is used twice in a > statement. ONLY essentially is a different kind of table object. Maybe I'm > missing something here. > >>> >>> During the course of implementing this, I found that the visit_delete >>> method did not call _compiler_dispatch on the table targeted by the delete, >>> preventing table aliases in a delete, and preventing my implementation of >>> ONLY for delete. I changed this, but the mssql dialect has some strange >>> aliasing rules, which needed some TLC to function correctly in the presence >>> of _compiler_dispatch. >> >> Also it seems a little awkward that DELETE now defers to generic compilation >> to format the table, but still not INSERT or UPDATE which still hardcode to >> preparer.format_table(). > For update this should probably be changed, and I've attached a patch to > do so. I'm not sure how much sense this makes for insert. I don't think you > can use anything but the name of the table (schema qualified) in an insert in > any DB. I do not believe that hints/aliases/ONLY could ever make sense in > this context. Maybe I'm missing something, besides the symmetry aspects of > it? > >>> >>> Of course, table aliasing in a delete isn't supported by all dialects, and >>> my current implementation doesn't do anything to protect Jimmy[1]. >> >> is aliasing of a table also a different feature need here ? Which DBs >> support this ? > As far as I know, it works on PostgreSQL and Firebird. It does not work > on SQLite, MSSQL, DB2, and Oracle. I have not tried on Sybase. > >>> >>> So there are two patches attached to this email, the first being for >>> _compiler_dispatch in visit_delete (compiler_dispatch_deletes.patch) and >>> the other for FROM ONLY in postgres (from_only.patch). The second one could >>> probably be considering more of a work-in-progress and I'm actually >>> interested in feedback on whether or not I'm headed in the right direction >>> with it. It also depends on the first patch. >> >>> >>> Also, is this the right list? >> >> >> sure, there's also sqlalchemy-devel, though for features/patches you can also >> use trac tickets or bitbucket forks/pull requests.... >> >> thanks for the patches and interest ! > Welcome. Sorry it took so long to follow up on this, I've been busy. > > -Ryan Kelly > >> >> >> >> >> >>> >>> -Ryan Kelly >>> >>> [1] http://www.globalnerdy.com/2010/05/09/new-programming-jargon/ >>> >>> -- You received this message because you are subscribed to the Google >>> Groups "sqlalchemy" group. To post to this group, send email to >>> sqlalchemy@googlegroups.com. To unsubscribe from this group, send email to >>> sqlalchemy+unsubscr...@googlegroups.com. For more options, visit this >>> group at http://groups.google.com/group/sqlalchemy?hl=en. >>> >>> <compiler_dispatch_deletes.patch><from_only.patch> >> >> -- You received this message because you are subscribed to the Google Groups >> "sqlalchemy" group. To post to this group, send email to >> sqlalchemy@googlegroups.com. To unsubscribe from this group, send email to >> sqlalchemy+unsubscr...@googlegroups.com. For more options, visit this group >> at http://groups.google.com/group/sqlalchemy?hl=en. >> > > -- > You received this message because you are subscribed to the Google Groups > "sqlalchemy" group. > To post to this group, send email to sqlalchemy@googlegroups.com. > To unsubscribe from this group, send email to > sqlalchemy+unsubscr...@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/sqlalchemy?hl=en. > > <compiler_dispatch_deletes.patch><from_only.patch>
with_only_hint.patch
Description: Binary data
-- You received this message because you are subscribed to the Google Groups "sqlalchemy" group. To post to this group, send email to sqlalchemy@googlegroups.com. To unsubscribe from this group, send email to sqlalchemy+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/sqlalchemy?hl=en.
