impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4355: random query generator: modify statement execution flow to support DML
Date Fri, 09 Dec 2016 22:07:50 GMT
Taras Bobrovytsky has posted comments on this change.

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


Patch Set 1:

(9 comments)

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


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


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 text in vim? set
textwidth=72 for example, http://stackoverflow.com/questions/823754/how-can-i-wrap-text-to-some-length-in-vim


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 already have a table
object passed in to this function?


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 already exact?


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 this be slow, or
are we planning to use small tables for testing?


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, you won't forget
to set the mode when creating the object.


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?


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


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