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 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/23816/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/23816/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java@673 PS3, Line 673: partitioned We don't check the number of partition columns here. Please add the check for it and revise the error message, i.e. non-HDFS table or HDFS table but non-partitioned. http://gerrit.cloudera.org:8080/#/c/23816/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java@1006 PS3, Line 1006: private static List<TPartitionKeyValue> parsePartitionName( Please move this static util method to FeCatalogUtils since there are other util methods dealing with partition names. I think we are lack of unit tests for this method. http://gerrit.cloudera.org:8080/#/c/23816/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java@1013 PS3, Line 1013: String[] parts = partitionName.split("/"); How do we handle the case when the partition values have "/" in the string? E.g. what's the correct URL to fetch the following partition? create table date_str_part_tbl (i int) partitioned by (ds string); insert into date_str_part_tbl partition(ds="12/25/2025") values (0),(1),(2); http://gerrit.cloudera.org:8080/#/c/23816/3/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/3/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@1072 PS3, Line 1072: Test IMPALA-9935 nit: we usually add JIRA id if this is added for a bug fix. For new features we usually don't add the JIRA id. http://gerrit.cloudera.org:8080/#/c/23816/3/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@1075 PS3, Line 1075: testGetPartitionCatalogObject_ValidPartitions nit: use shorter names like testGetValidPartitions, testGetNonExistingPartition, testGetNonExistingTablePartition. http://gerrit.cloudera.org:8080/#/c/23816/3/fe/src/test/java/org/apache/impala/catalog/CatalogTest.java@1158 PS3, Line 1158: public void testGetPartitionCatalogObject_MissingEquals() throws CatalogException { The test codes are simple and similar. Can we refactor some of them into one method using a for-loop with different strings? http://gerrit.cloudera.org:8080/#/c/23816/3/tests/webserver/test_web_pages.py File tests/webserver/test_web_pages.py: http://gerrit.cloudera.org:8080/#/c/23816/3/tests/webserver/test_web_pages.py@463 PS3, Line 463: cluster_properties) Can we add tests for multi-key partition names and partition values that have slashs "/" ? -- 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: 3 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, 12 Jan 2026 12:10:17 +0000 Gerrit-HasComments: Yes
