impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6160: Rework query execution to handle multiple statements in a Query object.
Date Sat, 11 Nov 2017 04:02:29 GMT
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 <twood@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Matthew Mulder <mmulder@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Tim Wood <twood@cloudera.com>
Gerrit-Comment-Date: Sat, 11 Nov 2017 04:02:29 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message