Marcel Kornacker has posted comments on this change. Change subject: Review Only: Kudu Impala integration ......................................................................
Patch Set 6: (10 comments) http://gerrit.cloudera.org:8080/#/c/1403/6/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: Line 664: opt_with_clause:w KW_INSERT KW_OVERWRITE opt_ignore:ignore > ok, created IMPALA-3145 my point wasn't purely that 'ignore' should be the default, but simply saying 'ignore' doesn't point out what is being ignored. i think this should be 'ignore duplicates'. http://gerrit.cloudera.org:8080/#/c/1403/6/fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java File fe/src/main/java/com/cloudera/impala/analysis/CreateTableStmt.java: Line 355: if (!KuduTable.tableParamsAreValid(getTblProperties())) { > left a TODO will mark for a follow up. where? if you leave todos in the code, please mark them so we can find them again (TODO(kudu-merge)?). http://gerrit.cloudera.org:8080/#/c/1403/6/fe/src/main/java/com/cloudera/impala/analysis/DistributeParam.java File fe/src/main/java/com/cloudera/impala/analysis/DistributeParam.java: Line 100: public void analyze(Analyzer analyzer) throws AnalysisException { > for this and for the relevant comments below, which require a refactor of t works for me. what's the jira? http://gerrit.cloudera.org:8080/#/c/1403/6/fe/src/main/java/com/cloudera/impala/planner/KuduScanNode.java File fe/src/main/java/com/cloudera/impala/planner/KuduScanNode.java: Line 114: List<LocatedTablet> tabletLocations = > this is, but we create a new client above, so we do this rpc everytime. we is there a jira for that? Line 221: * of the result can be evaluated with Expr::GetValue(NULL)). > done for the most part. the side effect (that the predicates are no longer yes http://gerrit.cloudera.org:8080/#/c/1403/6/fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java File fe/src/main/java/com/cloudera/impala/service/CatalogOpExecutor.java: Line 984: handler.dropTable(); > we have the NoOpDelegate for everything non-kudu meaning this will go through without an error. i don't know if it should be 'no-op'; 'UnsupportedOp' might be more appropriate. Line 1306: * available for the table, returns a NoOpDelegate instance. > agreed, probably not here though. Any specific action item you're recommend we should clean up how hbase is hardwire by moving the logic into the delegate. leave todo and file jira. Line 1309: if (KuduDdlDelegate.canHandle(tab)) { single line http://gerrit.cloudera.org:8080/#/c/1403/6/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java: Line 1617: "range(a) split rows ((1),('abc'),(3)) " + > Done please keep this test case, but as a commented-out AnalysisError. http://gerrit.cloudera.org:8080/#/c/1403/6/testdata/workloads/functional-query/queries/QueryTest/kudu-scan-node.test File testdata/workloads/functional-query/queries/QueryTest/kudu-scan-node.test: Line 24: DROP TABLE IF EXISTS impala_2740; why not drop? -- To view, visit http://gerrit.cloudera.org:8080/1403 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I239314acbc8434ef673a3a59d2a82a0338ea5876 Gerrit-PatchSet: 6 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Martin Grund <[email protected]> Gerrit-Reviewer: Silvius Rus <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
