Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12221 )
Change subject: IMPALA-5872: Testcase builder for query planner ...................................................................... Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/analysis/HdfsUri.java File fe/src/main/java/org/apache/impala/analysis/HdfsUri.java: http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/analysis/HdfsUri.java@79 PS7, Line 79: boolean registerPrivReq, boolean throwIfNotExists) throws AnalysisException { throwIfNotExists --> required Change meaning from implementation action to semantic requirement. Table must exist (else we throw an error.) http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@1173 PS7, Line 1173: "supported in a single expression: " + conjunct.toSql()); This check was already done elsewhere with an analysis exception? If so, let's not address the message to the user, let's make it clear that this is a developer error: an invariant is violated. Maybe, "Analysis failed to detect multiple subqueries". http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@262 PS7, Line 262: */ Somewhere here (you decide), would be good to explain the two modes now supported: "production" mode and "mock" mode backed by Derby for loading test cases. If no class is a good place to describe this, consider adding a package-info.java with the information. http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@395 PS7, Line 395: IOUtils.copyBytes(in, out, fs.getConf(), true); close file? Maybe try-with-resources to ensure the file is closed regardless of errors? http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@402 PS7, Line 402: JniUtil.deserializeThrift(testCaseData, decompressedBytes); Can the above fail for malformed files (bytes corrupted during transit, say? File truncated?) Should the above be wrapped in try/catch so we can explain that the file is, indeed, corrupted? http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@411 PS7, Line 411: if (ret != null) { Polarity wrong? addDb returns the old value. If the old value is null, then there was no DB and we just added one. If non-null, we replaced an existing copy (which should have come from the test case or we've got a muddle.) Indeed, should we fail if the DB already exists? Does the test case file ensure that the DB appears only once? So that, if we add a DB, and one already exists, we have a name collision with a "real" DB? http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@423 PS7, Line 423: catalog_.addTable(db, t); Any checks needed here for the case where the test case someone omitted a required DB? The general rule is: if it is externally-provided file, assume it is corrupted until proven otherwise by a successful load. http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@424 PS7, Line 424: t.getLock().lock(); Is this lock needed? We just added the table. Can't we check the table type before adding it, and increment the counters after? Or, since any error in addTable will fail this whole method, any harm in incrementing counters before adding and just ignoring the incomplete/wrong counters on error? Also, should we track the DB/table offsets/names and add them to any failure exception so we know where to look in the file for any problems? "Failed to load table 5: footer -- table already exists" http://gerrit.cloudera.org:8080/#/c/12221/7/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@441 PS7, Line 441: numTblsAdded, numViewsAdded)); %s --> %d for integers. Maybe "Loaded test case from Impala version %s: dbs: %d, tables: %d, views: %d" -- To view, visit http://gerrit.cloudera.org:8080/12221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec83eeb2dc5136768b70ed581fb8d3ed0335cb52 Gerrit-Change-Number: 12221 Gerrit-PatchSet: 7 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Balazs Jeszenszky <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Thu, 07 Feb 2019 00:06:59 +0000 Gerrit-HasComments: Yes
