Alex Behm has posted comments on this change. Change subject: IMPALA-4283: Ensure Kudu-specific lineage and audit behavior ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/5151/1/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java File fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java: Line 689: if (mentionedColumns.isEmpty()) { Can we reverse the logic as follows: if (tbl instanceof KuduTable) { List<String> mentionedColumns = insertStmt.getMentionedColumns(); Preconditions.checkState(!mentionedColumns.isEmpty()); ... } else { ... } That way the Kudu-specific nature seems clearer. Once this logic is reversed, it almost seems simpler to inline this at the single caller (which already has a similar instanceof KuduTable check). http://gerrit.cloudera.org:8080/#/c/5151/1/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: Line 795: for (int i = 0; i < resultExprs_.size(); ++i) { We don't really need resultExprs_ right? for (Integer i: mentionedColumns_) { result.add(columns.get(i).getName()); } http://gerrit.cloudera.org:8080/#/c/5151/1/fe/src/main/java/org/apache/impala/planner/Planner.java File fe/src/main/java/org/apache/impala/planner/Planner.java: Line 186: if (ctx_.isUpdateOrDelete()) return fragments; Does this line do anything? I though we established that this is an INSERT or CTAS in L180. I think we need to move this check somewhere else. http://gerrit.cloudera.org:8080/#/c/5151/1/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java File fe/src/test/java/org/apache/impala/analysis/AuditingTest.java: Line 374: public void TestKuduStatements() throws AuthorizationException, AnalysisException { nice! http://gerrit.cloudera.org:8080/#/c/5151/1/testdata/workloads/functional-planner/queries/PlannerTest/lineage.test File testdata/workloads/functional-planner/queries/PlannerTest/lineage.test: Line 4656: # Insert into a Kudu table with a column list specified Upsert Line 4661: "queryText":"insert into functional_kudu.alltypes (int_col, id) select int_col, id from\nfunctional.alltypes where id < 10", upsert? Line 4925: ==== What happens for DELETE and UPDATE statements? Might be worth adding a test. -- To view, visit http://gerrit.cloudera.org:8080/5151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idc4ca1cd63bcfa4370c240a5c4a4126ed6704f4d Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-HasComments: Yes
