Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22728 )
Change subject: IMPALA-13738 (Part2): Clean up code in Catalog's table and partition interfaces ...................................................................... Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/22728/2/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java File fe/src/main/java/org/apache/impala/catalog/FeFsTable.java: http://gerrit.cloudera.org:8080/#/c/22728/2/fe/src/main/java/org/apache/impala/catalog/FeFsTable.java@496 PS2, Line 496: for (FeFsPartition p: orderedPartitions) { > I like the idea of making this method shorter and delegating the task to Fe I actually liked the option of - "storing the long value in the TResultRowBuilder would not be nice and it might be overridden by an other addBytes() call." That's really the point though, imo. Anytime you add more bytes, you add more "totalCachedBytes". So "addBytes" actually should overwrite the variable as a tracking total. And while I never particularly love non-final variables, it's a "Builder" class, and every variable in the "Builder" class is non-final. But as I mentioned, I would create a wrapper "addCachedBytes" method that calls "addBytes" but that's where the totalCachedBytes gets +='ed. public TResultRowBuilder addCachedBytes(long val) { totalCachedBytes += val; return addBytes(val); } I won't hold up the code review if you don't think this is a good idea, but that's the way I would implement it. http://gerrit.cloudera.org:8080/#/c/22728/2/fe/src/main/java/org/apache/impala/catalog/Table.java File fe/src/main/java/org/apache/impala/catalog/Table.java: http://gerrit.cloudera.org:8080/#/c/22728/2/fe/src/main/java/org/apache/impala/catalog/Table.java@135 PS2, Line 135: protected SqlConstraints sqlConstraints_ = new SqlConstraints(new ArrayList<>(), > @Steve, there is a 'newbie' and a 'ramp-up' label you can put on the Jira. I suppose it's safer to create a new SqlConstraints everytime, but Idk...I would still create a static empty one and return that. But I'm ok as/is. -- To view, visit http://gerrit.cloudera.org:8080/22728 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d9c7ba876e39fa4f4d067761f25dc4ecfd55702 Gerrit-Change-Number: 22728 Gerrit-PatchSet: 2 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, 08 Apr 2025 14:05:19 +0000 Gerrit-HasComments: Yes
