Bharath Vissapragada 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) Thanks for your comments. I need some clarifications before I can send a new patch out. Can you please check and answer. 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 I'm not sure I got your comment. Can you please clarify? 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, le I think preconditions check failure generally means that an invariant is violated. Anyway, prefixed with "Invariant violated:" 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 sup Done. Added in MetastoreClientPool class (since that is what differentiates the two implementations). 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? The last boolean 'true' is to close the streams. Clarified that with inline comment. https://hadoop.apache.org/docs/r2.6.3/api/org/apache/hadoop/io/IOUtils.html#copyBytes(java.io.InputStream,%20java.io.OutputStream,%20org.apache.hadoop.conf.Configuration,%20boolean) 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 Good point, done, 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 I commented about "clashes" below. The way this is implemented, we don't have clashes here. Btw, this is how addDb() is. It returns the newDb, not the old one? public Db addDb(String dbName, org.apache.hadoop.hive.metastore.api.Database msDb) { Db newDb = new Db(dbName, msDb); versionLock_.writeLock().lock(); try { newDb.setCatalogVersion(incrementAndGetCatalogVersion()); addDb(newDb); return newDb; } finally { versionLock_.writeLock().unlock(); } } 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 r you mean check if the db exists? If so, yes I added a preconditions check that the DB should've been added by now (in the next PS) 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 The lock is actually annoying. We need that because we have an assertion in toThrift() (called by toTCatalogObject()) that checks that the current thread holds this lock [1]. That was introduced because we had some issues back in the day when toThrift() was called from multiple threads which caused some corruption. Btw, I reduced the scope of the lock only to toTCatalogObject() and updated the counters. Coming to the name clashes, the behavior is a little subtle (updated the comments now). So the "add"s here are overwrites. If a table or db with the same name exists, we overwrite them with these new objects. However the catch is that, these overwrites are not persisted into HMS. So something like INVALIDATE METADATA should get the old state back. This way, we don't need to worry about name clashes if someone loads a same testcase file twice (back to back) and they need to worry about dropping all the dbs/tbls etc. Lemme know what you think. [1] https://github.com/apache/impala/blob/master/fe/src/main/java/org/apache/impala/catalog/Table.java#L352 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. Ouch, old habits die hard. -- 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 <bhara...@cloudera.com> Gerrit-Reviewer: Balazs Jeszenszky <jes...@gmail.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Greg Rahn <gr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Comment-Date: Thu, 07 Feb 2019 04:19:10 +0000 Gerrit-HasComments: Yes
