|
|
Patch Set 1 #
Total comments: 14
Patch Set 2 : allow use of tree as first element #
Total comments: 3
Patch Set 3 : Use truncate instead of delete #Patch Set 4 : Use truncate from python-sql #
Total comments: 1
Patch Set 5 : Remove reference to truncate #Patch Set 6 : Add ignored branches #
Total messages: 15
|
nicoe
|
12 years, 7 months ago (2013年06月07日 14:28:32 UTC) #1 | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
https://codereview.appspot.com/9943044/diff/1/setup.py File setup.py (right): https://codereview.appspot.com/9943044/diff/1/setup.py#newcode14 setup.py:14: description='Library to access Tryton server as a client', in README I read: A library to create pre-computed tables
https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py File hebi/__init__.py (right): https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode14 hebi/__init__.py:14: class ScalarColumn(sql.Column): Could be sql.Literal https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode32 hebi/__init__.py:32: class flat(Column): Flat https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode36 hebi/__init__.py:36: class relation(flat): Relation https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode43 hebi/__init__.py:43: class tree(Column): Tree https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode87 hebi/__init__.py:87: class leaf(Column): Leaf https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode95 hebi/__init__.py:95: def __init__(self, prefix='', *columns): I would separate grouping columns and leaf because leaf should always be at the end. So having this signature sounds less error prone: Schema(grouping=(), aggregate=(), prefix='') https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode106 hebi/__init__.py:106: "{} should be a instance of Column".format(name)) an instance https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode107 hebi/__init__.py:107: self.cumulate = all(issubclass(l.operator, CUMULATABLE_AGGREGATORS) It should be better to have it as a property in case leafs is modified after the instantiation. https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode110 hebi/__init__.py:110: def groupbys(self): groupbys -> groupings https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode113 hebi/__init__.py:113: while level < len(keys): for i, key in enumerate(keys): yield keys[:len(keys) - i] https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode127 hebi/__init__.py:127: def explode_table(self, table, cursor): delete should always be run before any processing. https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode132 hebi/__init__.py:132: if isinstance(self.branches[groupby[-1]], tree): I'm wondering if not tree could not be a simple case of tree? https://codereview.appspot.com/9943044/diff/1/hebi/__init__.py#newcode183 hebi/__init__.py:183: for col_name in groupby + self.leafs.keys()] This construction is based on the specific way of constructing create_groupby_query. I find having both in separate methods dangerous for maintenance to keep consistency.
https://codereview.appspot.com/9943044/diff/15001/hebi/__init__.py File hebi/__init__.py (right): https://codereview.appspot.com/9943044/diff/15001/hebi/__init__.py#newcode161 hebi/__init__.py:161: print parents print statement https://codereview.appspot.com/9943044/diff/15001/hebi/__init__.py#newcode184 hebi/__init__.py:184: print '+' * 80 print stamenet https://codereview.appspot.com/9943044/diff/15001/hebi/__init__.py#newcode189 hebi/__init__.py:189: print tuple(update_qry) print stament
Right now Hebi is not Python 2.6 compatible, while this is stated in setup.py. https://codereview.appspot.com/9943044/diff/47001/hebi/__init__.py File hebi/__init__.py (right): https://codereview.appspot.com/9943044/diff/47001/hebi/__init__.py#newcode97 hebi/__init__.py:97: self.leafs = collections.OrderedDict() According to setup.py Hebi is supposed to be Python 2.6 compatible. OrderedDict is not in collections in 2.6, but there's a drop-in replacement package available [1] I always patch hebi by adding "from collections OrderedDict" at the top unless an ImportError is raised, and then try "from ordereddict import OrderedDict". And I change all "collection.OrderedDict" references to just "OrderedDict". [1] https://pypi.python.org/pypi/ordereddict
I think the tree column process could be replaced by a recursive SQL query that flatten the tree by duplicating each leaf for all his grand-parents. With this kind of query the code of explode_table could be unified and simplier.