impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <>
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. ( )

Change subject: IMPALA-6160: Rework query execution to handle multiple statements in a Query

Patch Set 1:


Thanks for taking this on!
Commit Message:
PS1, Line 7: IMPALA-6160: Rework query execution to handle multiple statements in a Query
nit: For the subject line, please keep it under 72 characters and one line. Maybe "Allow multiple
statements in a query object"?
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.
File tests/performance/
PS1, Line 45: query options' names
I'm a bit confused about this - I don't see query options in these regexes.
PS1, Line 52: SELECT
SELECT starts statements that are not DDL or DML, yes?
PS1, Line 52: re.I
I think spelling out IGNORECASE makes it much more readable.
PS1, Line 186:   """Executes a query.
Can you change the comments and member variable to pluralize "query"?
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?
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?
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
             :     test.
nit: I think you can omit this.

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Matthew Mulder <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Tim Wood <>
Gerrit-Comment-Date: Sat, 11 Nov 2017 04:02:29 +0000
Gerrit-HasComments: Yes

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