Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11928 )
Change subject: IMPALA-7839: Remove code duplication for getting a unique catalog object name ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java File fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java: http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@45 PS2, Line 45: return Catalog.toCatalogObjectKey(toTCatalogObject()); > This is a fair point. Initially I thought of doing that, too. Given the cur Doesn't something like the following work? Just an example for DataSource but other cases are similar too. Do we need to do something extra? public static String toCatalogObjectKey(TCatalogObject catalogObject) { Preconditions.checkNotNull(catalogObject); switch (catalogObject.getType()) { ....... case DATA_SOURCE: return DataSource.fromThrift(catalogObject.getData_source()).getUniqueName(); ........... } http://gerrit.cloudera.org:8080/#/c/11928/2/fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java@58 PS2, Line 58: protected abstract void writeToCatalogObject(TCatalogObject catalogObject); > I know this is the convention that we in some places, however I'm kind of t Ya, the compile time enforcement is missing unless we separate it out. How about renaming it to setTCatalogObject()? to be consistent with thrift's set*()? Also could you add a little more detail in the method doc. Mention that it is used to toTCatalog..()? -- To view, visit http://gerrit.cloudera.org:8080/11928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8218e57210c06fe6f2578a792d518e4b0287f15f Gerrit-Change-Number: 11928 Gerrit-PatchSet: 2 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 21 Nov 2018 17:49:27 +0000 Gerrit-HasComments: Yes
