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 think the problem with 'totalCachedBytes' is that we may use TResultRowBu Not sure I understand this critique. If there are multiple bytes columns, you have the choice of calling addCachedBytes or addBytes directly (without adding to the totalCachedBytes variable). If the developer chooses wrong, that's the developer's (and code reviewer's) fault. And they can still screw it up if they put totalCachedBytes outside the class as well. 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<>(), > I also think that returning new objects is safer. Then we should make SqlPrimaryKey and SqlForeignKey immutable too (if they aren't already, I haven't checked) :) File a jira then to move the object to HdfsTable then? -- 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: Wed, 09 Apr 2025 15:43:13 +0000 Gerrit-HasComments: Yes
