Daniel Becker 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) { > Not sure I understand this critique. 'totalCachedBytes' does not seem semantically right to me because its relation to the schema(s) is unclear. When using TResultRowBuilder, the schema can be anything, with any number of "byte" typed columns. Which column(s) belong(s) to the cached value? Or should we add as many cached values as there are byte-typed columns? I think it would add unnecessary complexity. I think even if we were to extract parts of this function to another function of HdfsPartition, it would still be cleaner to return "totalCachedBytes" in a Pair<>. 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<>(), > Then we should make SqlPrimaryKey and SqlForeignKey immutable too (if they -- Then we should make SqlPrimaryKey and SqlForeignKey immutable too Unfortunately they are Thrift structs, I'm not sure it's possible to make those immutable. -- 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: Fri, 11 Apr 2025 11:47:39 +0000 Gerrit-HasComments: Yes
