Casey Ching has posted comments on this change. Change subject: Misc query generator improvements ......................................................................
Patch Set 2: (13 comments) http://gerrit.cloudera.org:8080/#/c/2445/2/tests/comparison/discrepancy_searcher.py File tests/comparison/discrepancy_searcher.py: Line 294: mem_limit=randint(1024 ** 3, 10 * 1024 ** 3), > Maybe it's better to move all these constants into query_profile? Not sure yet. The stress test should do something like this too. I was going to figure it out later. Line 336: self.set_impala_query_optons(cursor) > I think we should not be setting the options all the time. How about adding Same as above. I didn't want to put much effort into this yet. Line 527: def search(self, number_of_test_queries): > I'm curious, did you manage to find many front end bugs with this? Nothing new. I mostly used this to check that queries were valid rather than running query_generator.py. It's still pretty fast and actually checks that the query is valid. http://gerrit.cloudera.org:8080/#/c/2445/2/tests/comparison/query_generator.py File tests/comparison/query_generator.py: Line 402 > It looks like null_args are overwritten below so this line is pointless. Wa Yes this is #3 in the commit message Line 395: if func.contains_subquery: > how about: I think the problem was that doesnt fit on one line. Line 438: only_accepts > how about accepts_only? Done Line 1193: assert possible_left_table_exprs > Why do we need an assert here? What happens if it fails? Its just a dev check. If this fails an error is thrown. Line 1237: None > why not set this to 0? Done Line 1246: arg_stop_idx = len(func.args) > arg_stop_ids = arg_stop_idx or len(func.args) That's functionally different. In theory someone could call the function with start/stop = 0/0. Line 1250: if null_args is None: > null_args = null_args or list() This doesn't seem any better so I left it as is. One line is nice but its slightly harder to read and does extra work if the list is empty. This seems really minor either way. Line 1443: if not root_func: > how about changing order? (to get rid of the not) Done http://gerrit.cloudera.org:8080/#/c/2445/2/tests/comparison/query_profile.py File tests/comparison/query_profile.py: Line 436: and not any(a for a in s.args if a.is_subquery), > how about: Done Line 492: in > I prefer if signature.func not in func_weights Done -- To view, visit http://gerrit.cloudera.org:8080/2445 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I75a7e6a1d96e5d92368f73c1e5e6a6b288932497 Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Casey Ching <[email protected]> Gerrit-Reviewer: Casey Ching <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
