Michael Brown has posted comments on this change. Change subject: IMPALA-3864: qgen: reduce likelihood of create_query() exceptions ......................................................................
Patch Set 3: (4 comments) http://gerrit.cloudera.org:8080/#/c/3720/3/tests/comparison/query_generator.py File tests/comparison/query_generator.py: Line 1468: if __name__ == '__main__': > How about putting everything in below this line into a function called some Done Line 1488: col_idx = i * len(data_types) + type_idx > Maybe it would be more readable (and simpler) if we declared col_idx = 0 ou Done http://gerrit.cloudera.org:8080/#/c/3720/3/tests/comparison/query_profile.py File tests/comparison/query_profile.py: PS3, Line 90: And > does it make sense to have AND and OR here? It's already in conjunct_dijunc No, but the way the signature selection works, And and Or could be selected because they fit the criteria: 2 arguments and they return Boolean. We hit the condition in L457-458 if we don't define these. We don't have a more advanced way to pick and choose selections of signatures / functions. I don't believe we should implement that now. I've filed IMPALA-3896 for future consideration. Options: 1. Leave as-is, or add a note. 2. Leave And / Or defined in this dictionary but put their weights to 0 with a note saying they won't be selected. 3. Remove these here and do some sort of filtering to trim those signatures whenever we ask for a relational function signature. 4. Relax L458 to a warning, but no one will read it. :) Let me know what you think. Line 518: else: > How about putting this first? So change the if statement to if func_weights Done -- To view, visit http://gerrit.cloudera.org:8080/3720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idd9434a92973176aefb99e11e039209cac3cea65 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Michael Brown <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
