Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17812 )
Change subject: IMPALA-10888: getPartitionsByNames should return sorted partitions ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java File fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java: http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@459 PS2, Line 459: for (FieldSchema partSchema : partitionKeys) { > Assuming retPartitions list is non empty, should we assert that partitionKe Yes, I think that is a valid preconditions check given that this is a public static method and not a private method. http://gerrit.cloudera.org:8080/#/c/17812/2/fe/src/main/java/org/apache/impala/catalog/CatalogHmsAPIHelper.java@463 PS2, Line 463: (part) -> MetastoreShim.makePartName(partitionColNames, part.getValues()))); > If this is done only for testing. We can just have a helper utility method, Yes, I had initially thought of making this test only change. However, I thought it may be a better solution to simulate what the HMS is doing since we cannot be sure if HMS clients assume sorted order or not in the returned partitions. Currently, the order of partitions returned in case of a catalogd HMS call v/s direct HMS call is not the same which feels like a discrepancy to me. I guess we can keep the current implementation and modify the test until we see a real-world case where clients assume sorted partitions. Thoughts? -- To view, visit http://gerrit.cloudera.org:8080/17812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75aa9d714e888ec7f7efd62e475ba1d3a3342256 Gerrit-Change-Number: 17812 Gerrit-PatchSet: 2 Gerrit-Owner: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Anonymous Coward <kis...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sourabh Goyal <soura...@cloudera.com> Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com> Gerrit-Comment-Date: Fri, 27 Aug 2021 19:53:29 +0000 Gerrit-HasComments: Yes
