impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Wood (Code Review)" <ger...@cloudera.org>
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. ( http://gerrit.cloudera.org:8080/8513 )

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


Patch Set 2:

(7 comments)

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.
Residue; will correct.


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.
Ack


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?
I consider SELECT as DML (data manipulation, but not mutation. :)  I could call this DDL_CRUD_PATTERN
if it's that important.


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"?
Ack


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 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.


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 timi
The execution logic in query_exec_functions.py 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.


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.
Ack



-- 
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: 2
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: Mon, 13 Nov 2017 19:40:00 +0000
Gerrit-HasComments: Yes

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