impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Wood (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-6160: Rework query execution to handle multiple statements in a Query object.
Date Mon, 13 Nov 2017 19:40:00 GMT
Tim Wood has posted comments on this change. ( )

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

Patch Set 2:

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.
Residue; will correct.
PS1, Line 52: re.I
> I think spelling out IGNORECASE makes it much more readable.
PS1, Line 52: SELECT
> SELECT starts statements that are not DDL or DML, yes?
I consider SELECT as DML (data manipulation, but not mutation. :)  I could call this DDL_CRUD_PATTERN
if it's that important.
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 s
I don't want to limit the use cases to just SET, when there's nothing really unique about
it.  This routine would get more complicated(*), add needless error conditions, and hasten
the day when it would need changing again.  

* What if it's 2 SETs and a query?  A query, a SET and a query?  There's no need to reject
these legal batches.  And, it turns out, the performance harness assumes results are correct
without checking.
PS1, Line 229: timing
> How DOES the timing work for this object? There doesn't seem to be any timi
The execution logic in sets the start_time and time_taken fields of
the various QueryResult objects.  The loop at l. 256 below does overwrite the reference to
the query result for statements 1..n-1 and leaves the result of the last as the result of
the batch.  I will document this.  So far, there's no requirement for the higher layers to
receive a list of results for the batch.
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: 2
Gerrit-Owner: Tim Wood <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Matthew Mulder <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Tim Wood <>
Gerrit-Comment-Date: Mon, 13 Nov 2017 19:40:00 +0000
Gerrit-HasComments: Yes

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