impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4355: random query generator: modify statement execution flow to support DML
Date Tue, 13 Dec 2016 00:29:46 GMT
Michael Brown has posted comments on this change.

Change subject: IMPALA-4355: random query generator: modify statement execution flow to support
DML
......................................................................


Patch Set 1:

(10 comments)

Thanks for the reviews. Patch set 2 has been updated to address changes. Patch set 3 is a
rebase.

http://gerrit.cloudera.org:8080/#/c/5387/1//COMMIT_MSG
Commit Message:

PS1, Line 11: which
> wording: randomly choose a table on which to execute
Done


PS1, Line 12:  copied to run the DML statement 
> I'm not really following the logic. We randomly choose a table because we n
Done


Line 36:   modes. The first is DML_SETUP: this is a DML statement that needs to be
> long line. By the way, did you know that there is an automatic way to wrap 
Done


http://gerrit.cloudera.org:8080/#/c/5387/1/tests/comparison/discrepancy_searcher.py
File tests/comparison/discrepancy_searcher.py:

Line 635:     # 1. Don't have to spend more time parsing SHOW CREATE TABLE output just to
get it
> This point is not very clear. Why do we need to parse anything, do we alrea
Done, because I assume you read further below that we're copying a table, and the new patch
set will describe motivations for doing so.


Line 637:     # 2. Ensure we copy the table-under-test exactly. This includes primary keys,
> It's not really clear to me either. Isn't the table object that's passed in
Done, because I assume you read further below that we're copying a table, and the new patch
set will describe motivations for doing so.


Line 695:         result = query_result_comparator.compare_query_results(table_copy_statement)
> So we copy the entire table for every query that we want to execute? Won't 
There are reasons for doing it this slower way for now. Added a comment.


http://gerrit.cloudera.org:8080/#/c/5387/1/tests/comparison/query.py
File tests/comparison/query.py:

Line 111:     self.execution = StatementExecutionMode.SELECT_STATEMENT
> Maybe better to set it to None by default, like other variables? This way, 
It's fine to leave it like this. The AbstractStatement has this set to None and the only consumer
of a Query object's execution attribute is the discrepancy searcher.


http://gerrit.cloudera.org:8080/#/c/5387/1/tests/comparison/statement_generator.py
File tests/comparison/statement_generator.py:

Line 32: class InsertStatementGenerator(object):
> Is the plan to also support all DML statements for this release?
I've not seen anything on the Apache Impala (incubating) mailing lists about another Apache
Impala (incubating) release proposal. I expect when the PMC decides to cut a release, whatever
is in here at the time will be part of that release. This patch is about changes to the discrepancy
searcher to support DML statements, and here we introduce a stub for INSERT statement generation.
Other Jiras track randomizing all statements for real, under the umbrella IMPALA-3740.


PS1, Line 47: dml_table is a required Table object. The INSERT will be into this table.
> Maybe this is a nit, but there are three main docstring styles the I know o
TODO filed as IMPALA-4656


PS1, Line 53: (_,
> since you're using the _ variable, you should name it something else.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5387
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4c63a2223185d0e056cc5713796772e5d1b8414
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message