impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <>
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


Thanks for the review. Please see patch set 7.
File tests/comparison/

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

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

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

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

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

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?

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
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I38e24da78c49e908449b35f0a6276ebe4236ddba
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Michael Brown <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message