Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/8513 )
Change subject: IMPALA-6160: Rework query execution to handle multiple statements in a Query object. ...................................................................... Patch Set 1: (9 comments) Thanks for taking this on! http://gerrit.cloudera.org:8080/#/c/8513/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8513/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-6160: Rework query execution to handle multiple statements in a Query object. nit: For the subject line, please keep it under 72 characters and one line. Maybe "Allow multiple statements in a query object"? http://gerrit.cloudera.org:8080/#/c/8513/1//COMMIT_MSG@14 PS1, Line 14: --query_iterations=3 --table_formats=parquet/none --plan_first --query_names='.*' nit: can you wrap at 72 characters, here? If you use emacs to write your commit message, you can wrap automatically with ctrl-q, though it might not understand your bullets and then rewrap those unhelpfully. http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py File tests/performance/query_executor.py: http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@45 PS1, Line 45: query options' names I'm a bit confused about this - I don't see query options in these regexes. http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@52 PS1, Line 52: SELECT SELECT starts statements that are not DDL or DML, yes? http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@52 PS1, Line 52: re.I I think spelling out IGNORECASE makes it much more readable. http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@186 PS1, Line 186: """Executes a query. Can you change the comments and member variable to pluralize "query"? http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@224 PS1, Line 224: a set of SQL statements This looks very general to me. Might you be able to get by with something simpler that only handles one SET followed by one EXPLAINable statement? http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@229 PS1, Line 229: timing How DOES the timing work for this object? There doesn't seem to be any timing code directly in this. Maybe it is done in self.exec_func storing the timing info somewhere? The EXPLAINs are done with self.exec_func as well, so I'd guess that every call to self.exec_func clobbers the old timing. If that's right, then executing multiple queries will lead to only a timing from the last call being stored, yes? http://gerrit.cloudera.org:8080/#/c/8513/1/tests/performance/query_executor.py@231 PS1, Line 231: : This function originally assumed that self.query contained only a single : query statement, but that assumption is not valid for a test file that : contains, e.g., a SET DECIMAL_V2=n; statement before the actual query for the : test. nit: I think you can omit this. -- To view, visit http://gerrit.cloudera.org:8080/8513 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac86af181b7c42655f21d2c1efd4652dd35d9297 Gerrit-Change-Number: 8513 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Wood <tw...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Matthew Mulder <mmul...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Tim Wood <tw...@cloudera.com> Gerrit-Comment-Date: Sat, 11 Nov 2017 04:02:29 +0000 Gerrit-HasComments: Yes
