Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14720 )
Change subject: IMPALA-9104: Support retrieval of PK/FK information through impala-hs2-server. ...................................................................... Patch Set 7: (13 comments) Btw, not sure if you're familiar with clang-format, some instructions here: https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala, but it can help a lot to reduce the number of formatting mistakes and make the review process easier http://gerrit.cloudera.org:8080/#/c/14720/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14720/7//COMMIT_MSG@38 PS7, Line 38: We should fix : this seperately. Is there a JIRA for it? http://gerrit.cloudera.org:8080/#/c/14720/7/common/thrift/Frontend.thrift File common/thrift/Frontend.thrift: http://gerrit.cloudera.org:8080/#/c/14720/7/common/thrift/Frontend.thrift@576 PS7, Line 576: GET_CROSS_REFERECE GET_CROSS_REFERENCE http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/analysis/TableDef.java@547 PS7, Line 547: nit: extra whitespace http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/FeTable.java File fe/src/main/java/org/apache/impala/catalog/FeTable.java: http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/FeTable.java@99 PS7, Line 99: throws NotImplementedException; I don't think this is necessary, here and elsewhere http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java: http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java@56 PS7, Line 56: import org.apache.impala.common.NotImplementedException; I don't think you need this http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java: http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@216 PS7, Line 216: "getPrimaryKeys() is not yet implemented in " : + "LocalCatalog Mode" nit: you can get rid of the '+' by putting this all on the second line http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/Frontend.java@907 PS7, Line 907: } catch (NotImplementedException e) { : // getForeignKeys() is not supported in LocalCatalog mode. Until IMPALA-9158 is : // fixed, return an empty list. : return Lists.newArrayList(); My thinking in suggesting the NotImplementedException was that we would bubble it up to users so that they know that its not something that's expected to work yet. I'm not certain that would actually work, eg. because of the way this is getting called through JNI, and I suppose it doesn't really matter since I see that you've already got a patch out for IMPALA-9158. Feel free to leave this the way it is, just as long as you clean it up in that patch. http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java File fe/src/main/java/org/apache/impala/service/MetadataOp.java: http://gerrit.cloudera.org:8080/#/c/14720/5/fe/src/main/java/org/apache/impala/service/MetadataOp.java@736 PS5, Line 736: Lists.newArrayList > Done. Just curious: How is this different from new ArrayList<>()? Good question - I just know if it as our usual convention, eg. if you look at the rest of this file. Apparently though Lists.newArrayList() is supposed to be deprecated as of Java 7, see: https://guava.dev/releases/19.0/api/docs/com/google/common/collect/Lists.html#newArrayList() and the way you had it previously is now considered the preferable way Of course, it can be a bit of a judgement call whether to go with the "preferred" style or to match the existing style in the file you're modifying, but some quick grepping suggests that we already use the "diamond" style more overall across the FE than Lists.newArrayList, so you can feel free to put this back the way that it was. http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/MetadataOp.java File fe/src/main/java/org/apache/impala/service/MetadataOp.java: http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/MetadataOp.java@650 PS7, Line 650: if (LOG.isTraceEnabled()) LOG.trace("Returning {} primary keys for table {}.", : result.rows.size(), tableName); formatting http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/MetadataOp.java@676 PS7, Line 676: , user formatting http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/main/java/org/apache/impala/service/MetadataOp.java@720 PS7, Line 720: if (LOG.isTraceEnabled()) LOG.trace("Returning {} foreign keys for table {}.", : result.rows.size(), foreignTableName); formatting http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java File fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java: http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java@348 PS7, Line 348: previleges typo http://gerrit.cloudera.org:8080/#/c/14720/7/fe/src/test/java/org/apache/impala/authorization/AuthorizationTest.java@373 PS7, Line 373: nit: unnecessary whitespace -- To view, visit http://gerrit.cloudera.org:8080/14720 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8942dfbbd4a3be244eed1c61ac2ce17069960477 Gerrit-Change-Number: 14720 Gerrit-PatchSet: 7 Gerrit-Owner: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Anurag Mantripragada <anu...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Tue, 19 Nov 2019 19:46:04 +0000 Gerrit-HasComments: Yes
