Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/23816 )
Change subject: IMPALA-9935: Support individual partitions in catalog_object webUI page ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/23816/5/be/src/catalog/catalog-util.cc File be/src/catalog/catalog-util.cc: http://gerrit.cloudera.org:8080/#/c/23816/5/be/src/catalog/catalog-util.cc@186 PS5, Line 186: URL nit: The value string "12/25/2025" is not a URL so saying "it" to represent it is more clear. http://gerrit.cloudera.org:8080/#/c/23816/5/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java File fe/src/test/java/org/apache/impala/catalog/CatalogTest.java: http://gerrit.cloudera.org:8080/#/c/23816/5/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@1083 PS5, Line 1083: TCatalogObject partitionDesc = new TCatalogObject(); : partitionDesc.setType(TCatalogObjectType.HDFS_PARTITION); : THdfsPartition partInfo = new THdfsPartition(); : partInfo.setDb_name("functional"); : partInfo.setTbl_name("alltypes"); : partInfo.setPartition_name("year=2009/month=1"); : partitionDesc.setHdfs_partition(partInfo); : : TCatalogObject result = catalog_.getTCatalogObject(partitionDesc); : assertNotNull(result); : assertEquals(TCatalogObjectType.HDFS_PARTITION, result.getType()); : assertTrue(result.isSetHdfs_partition()); : assertEquals("functional", result.getHdfs_partition().getDb_name()); : assertEquals("alltypes", result.getHdfs_partition().getTbl_name()); : assertEquals("year=2009/month=1", result.getHdfs_partition().getPartition_name()); Extract these into a method and use it twice in this test. It can also be used in testGetPartitionWithSpecialCharacters. http://gerrit.cloudera.org:8080/#/c/23816/5/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@1109 PS5, Line 1109: /** : * Test non-existent partition should throw exception. : */ nit: remove simple and verbose comments like this that are already explained well by the method name. http://gerrit.cloudera.org:8080/#/c/23816/5/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@1123 PS5, Line 1123: TCatalogObject partitionDesc = new TCatalogObject(); : partitionDesc.setType(TCatalogObjectType.HDFS_PARTITION); : THdfsPartition partInfo = new THdfsPartition(); : partInfo.setDb_name("functional"); : partInfo.setTbl_name("nonexistenttable"); : partInfo.setPartition_name("year=2009/month=1"); : partitionDesc.setHdfs_partition(partInfo); : : try { : catalog_.getTCatalogObject(partitionDesc); : fail("Expected CatalogException for non-existent table"); : } catch (CatalogException e) { : assertTrue(e.getMessage().contains("Table not found")); : } Can we use testPartitionWithError() here? http://gerrit.cloudera.org:8080/#/c/23816/5/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@1146 PS5, Line 1146: catalog_.getOrLoadTable(dbName, tableName, "test", null); To refactor more codes, we need a method like this without loading the table. We can extract the remaining lines of this method into a new method, e.g. verifyPartitionError(), and correspondingly rename this method to loadAndVerifyPartitionError(). Then testGetPartitionFromInvalidatedTable and testGetPartitionFromFailedTable can use verifyPartitionError(). http://gerrit.cloudera.org:8080/#/c/23816/5/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@1148 PS5, Line 1148: TCatalogObject partitionDesc = new TCatalogObject(); : partitionDesc.setType(TCatalogObjectType.HDFS_PARTITION); : THdfsPartition partInfo = new THdfsPartition(); : partInfo.setDb_name(dbName); : partInfo.setTbl_name(tableName); : partInfo.setPartition_name(partitionName); : partitionDesc.setHdfs_partition(partInfo); There are still duplicated codes like these. Let's extract these to a method, e.g. createPartitionDescriptor(), -- To view, visit http://gerrit.cloudera.org:8080/23816 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5645a20283e664af12d04a9665c8870c7666a74c Gerrit-Change-Number: 23816 Gerrit-PatchSet: 5 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Mon, 19 Jan 2026 08:33:16 +0000 Gerrit-HasComments: Yes
