Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22605 )
Change subject: IMPALA-13738 (Part1): Denormalize IcebergTable.getPartialInfo() ...................................................................... Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/22605/1/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergTable.java: http://gerrit.cloudera.org:8080/#/c/22605/1/fe/src/main/java/org/apache/impala/catalog/IcebergTable.java@774 PS1, Line 774: if (partIds != null || wantPartitionInfo) { > @Steve Carlin Oh, so the helper function shouldn't change anything, which is a good thing (and glad you approved :) ) So as to your question about part.setPartitionMetadata(partInfo); Heh, yeah, I suppose HdfsPartition is really a representative a database object and these are constantly mutated. That's part of the architecture when dealing with database objects, so a "setter" function is ok. But I think I would still prefer the setter to be in the getPartialInfo() function and not the getPartialPartitionInfo() function. The private getPartialPartitionInfo() is now a pure immutable function that is responsible only for creating the thrift object. Keeping it clean of setter functions is a good thing, imo. The getPartialInfo() function is the request/response interface. Imo, if we put the setter in the private function, it's a little harder to find while debugging. And I would make a comment saying that we are manipulating the HdfsPartition database object, even if it is just the thrift variables. It's still an internal manipulation of the HdfsPartition object. Or now that I'm thinking about it and looking it at it more... If the plan is to eventually support other partitions? We really want to pass in a FeFsPartition type into this getPartialInfo() method rather than a partId. We can get the HdfsPartition on the outside. I looked at the methods you used for HdfsPartition...only toHmsPartition() is HdfsPartition specific (I think). For that one, I think you can put a precondition that it is an instance of HdfsPartition (since it relies on a specific want_hms_partition flag) and cast it to an HdfsPartition there. Whatcha think? -- To view, visit http://gerrit.cloudera.org:8080/22605 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75d277fb555d0088a2e28111ff529a4605d734fa Gerrit-Change-Number: 22605 Gerrit-PatchSet: 1 Gerrit-Owner: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Steve Carlin <scar...@cloudera.com> Gerrit-Comment-Date: Tue, 11 Mar 2025 21:38:22 +0000 Gerrit-HasComments: Yes
