David Ribeiro Alves has posted comments on this change. Change subject: Review Only: Kudu Impala integration ......................................................................
Patch Set 6: (9 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 > my point wasn't purely that 'ignore' should be the default, but simply sayi updated the comment on the jira 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())) { > where? if you leave todos in the code, please mark them so we can find them the TODO is in the method implementation. Makes sense, did this for the remaining TODO mentioned/created during this review too. 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 { > works for me. what's the jira? IMPALA-3156 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 = > is there a jira for that? there is now: IMPALA-3175 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(); > meaning this will go through without an error. i don't know if it should be renamed to UnsupportedOpDelegate Line 1306: * available for the table, returns a NoOpDelegate instance. > we should clean up how hbase is hardwire by moving the logic into the deleg created IMPALA-3176 for this Line 1309: if (KuduDdlDelegate.canHandle(tab)) { > single line Done 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)) " + > please keep this test case, but as a commented-out AnalysisError. Done 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? ?? aren't we dropping? -- 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
