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.
---
