When I first read the docs, what jumped right out at me was i found it confusing what the aggregation function would be - I see it sort of imitates how hybrids look, but then I noticed in one case it’s like:
@aggregated_attr
def net_worth(self):
return sa.Column(sa.Integer)
e.g. no function call on aggreagted_attr, then in the other it’s like:
@aggregated_attr('comments')
def comment_count(self):
return sa.Column(sa.Integer)
e.g. relationship name is passed, is that a typo ? I looked at the source of
@aggregated_attr and I think you need the () each time?
Once I looked at the source, it looked a little more familiar, e.g. the
aggregation function is passed and just defaults to func.count, OK, so I could
say:
@aggregated_attr(‘comments’, func.sum)
def comment_count(self):
return sa.Column(sa.Integer)
except that’s not very useful because it’s func.sum of the “.id” and not a
specific column, and I know we can’t just say func.sum(Comment.word_count)
because “Comment” isn’t available yet, sure. Maybe then there’s really no
reason for the “expression” to be passed there, as it already comes in via
.expression?
I thought about how there’s two functions here defined for any aggregate that
isn’t just item count, which seems off-balance compared to the default case of
“count”. I know that we want the expression to be anything, not just "func(some
column)”, which means it’s easier for that to be declared in a def, OK.
I thought, why wouldn’t it be this, since the destination is just a single
mapped Column (or is it?):
orders_sum = aggregated("orders", sa.Column(sa.Integer))
@orders_sum.expression
def orders_sum(self):
return sa.func.sum(Order.price)
or even more succinctly, we could lambda:
orders_sum = aggregated("orders”,
sa.Column(sa.Integer),
expression=lambda:
sa.func.sum(Order.price))
that particular syntax is consistent with what already works with
relationship() - e.g. you can use lambdas in order to have things evaluate
later at mapper configuration time (which is also what the “string” thing makes
use of):
orders = relationship(lambda: Order, primaryjoin=lambda: Order.customer_id
== Customer.id)
If I really didn’t like lambda, we could decorate like this perhaps:
@aggregated(“orders”, sa.Column(sa.Integer))
def orders_sum(self):
return sa.func.sum(Order.price)
I guess the two separate function calls is the part that’s seeming heavy to me
- overall it seems aggregated() here acts a bit like column_property.
Then i noticed in the source right now it’s taking advantage of being a
descriptor so that it can snatch the class and tack on “__aggregates__”. I’d
change that name to not be a dunder method…technically we’re not supposed to
use dunders at all as they’re supposed to be reserved for Python, even though I
know declarative has __table__, __mapper__, all that (I’d likely not have named
them that today), if it’s an attribute that isn’t “public” it should probably
be a single underscore name that won’t conflict with anything else (hence why I
use “._sa_XYZ” quite often outside of the few that made it into declarative).
More issues there, if we’re dealing with subclass inheritance, it looks like in
some cases the superclass and subclass will share the same __aggregates__
dictionary, and then the superclass will be exposed to aggregate attributes
that don’t apply to it, so that’s a bug - use “__aggregates__ in cls.__dict__”
instead of “hasattr()” to test for local presence of a class-specific
dictionary.
After some thought I think how I’d go with __aggregates__ is instead of tacking
it onto the class like that, just store “cls” in a module-level
WeakKeyDictionary, and then yank it out when you do the mapper_configured
event. that way you aren’t messing around with the class any more than you
have to, you don’t have to worry about ambiguity regarding
subclasses/superclasses, you don’t have to check for “attr in cls.__dict__” /
hasattr(cls, “__aggregates”) or worry about naming conflicts. Using weak
referencing registries is how I generally go with this sort of thing.
Looks like a great idea!
On Nov 11, 2013, at 4:37 AM, Konsta Vesterinen <[email protected]>
wrote:
> Hi all!
>
> I recently updated SQLAlchemy-Utils to support aggregated attributes. The
> solution is strongly influenced by RoR counter_culture. I'd love to hear what
> you guys think about this.
>
> - Konsta
>
> --
> You received this message because you are subscribed to the Google Groups
> "sqlalchemy" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to [email protected].
> To post to this group, send email to [email protected].
> Visit this group at http://groups.google.com/group/sqlalchemy.
> For more options, visit https://groups.google.com/groups/opt_out.
signature.asc
Description: Message signed with OpenPGP using GPGMail
