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 <mi...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-HasComments: Yes
