madlib-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From iyerr3 <...@git.apache.org>
Subject [GitHub] madlib pull request #195: Feature: Add grouping support to HITS
Date Mon, 13 Nov 2017 19:18:43 GMT
Github user iyerr3 commented on a diff in the pull request:

    https://github.com/apache/madlib/pull/195#discussion_r150637914
  
    --- Diff: src/ports/postgres/modules/utilities/utilities.py_in ---
    @@ -709,6 +709,17 @@ def _check_groups(tbl1, tbl2, grp_list):
         return ' AND '.join([" {tbl1}.{i} = {tbl2}.{i} ".format(**locals())
                              for i in grp_list])
     
    +def get_ignore_groups(first_table, second_table, grouping_cols_list):
    --- End diff --
    
    This function (and the `_grp_from_table`) are difficult to understand just from the names
(for eg. what does `first_table` and `second_table` impy?). Further, these are returning specific
SQL expressions - is there value in putting these in a general `utilities` module? If yes,
then I would suggest making the use case more general. 
    The `_grp_from_table` especially seems like extra work for a one-liner, without getting
benefits of abstraction. 


---

Mime
View raw message