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-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model
Date Wed, 30 Nov 2016 17:28:21 GMT
Michael Brown has posted comments on this change.

Change subject: IMPALA-4343,IMPALA-4354: qgen: model INSERTs; write INSERTs from query model
......................................................................


Patch Set 7: Code-Review+1

(8 comments)

Thanks for the review. Please see patch set 7.

http://gerrit.cloudera.org:8080/#/c/5162/5/tests/comparison/model_translator.py
File tests/comparison/model_translator.py:

Line 105:   def _write_query(self, query):
> Where does _write_query get used?
Done


PS5, Line 430: mn_lis
> How about changing the order of if and else to get rid of the not:
Done


http://gerrit.cloudera.org:8080/#/c/5162/5/tests/comparison/query.py
File tests/comparison/query.py:

PS5, Line 35: # reference
> It's a good idea to add a comment explaining what each variable means.
Done


Line 40:     self.with_clause = None
> I think it's kind of strange to have both @abstractproperty and the impleme
Done


Line 50:     abstract as the clauses that do this differ across query types. Since all supported
> return self.with_clause.table_exprs if self.with_clause else []
Done


PS5, Line 61: turns
> I think the word query is not the best choice any more. Let's call this Sel
Wontfix, but left an explanation as to why the name should stay the same.


Line 84:     self.group_by_clause = None
> why a blank line?
Done


PS5, Line 661: f __deepcopy__(self, memo):
> Can you explain in more detail what this is? (e.g. is it a list? what are t
Done. It's called "column permutation" but buried deep in docs http://www.cloudera.com/documentation/enterprise/latest/topics/impala_insert.html.
I renamed it "column_list" to match the grammar at the top, instead, and that gives a clearer
picture of what it is anyway.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message