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) { > 'totalCachedBytes' does not seem semantically right to me because its relat Heh, I think we'll have to agree to disagree here. The way the code is written as/is, it's an easy fit. To your first point of unnecessary complexity: The whole purpose of each iteration of this for loop is really just to create something. It adds too much bulk to this method and the idea of creating something in a for loop is a red flag to me that it needs to be within a method. Each iteration specifically deals with a partition...so it really seems logical to put it in the partition object. So, imo, if you are returning a Pair, you're really losing the battle here. Returning a Pair means you're creating a method that is slightly overloaded in some way. Functions should only return one thing, and we're making the function return 2 things. That's why it always seems clunky to me. Not gonna say I don't do it. Just that it always makes me cringe. So to resolve the problem of returning Pair, you can do one of 2 things: 1) Create a separate function that does what you need it to do to return the second object of the Pair. That doesn't work here because you'd have to go through the inner for loop twice and that would be ugly 2) Create a wrapper class that contains both parts of the Pair and return that one class. The reason I wind up using Pair sometimes is when the 2 parts of the Pair are big existing classes with lots of members, and disjoint. And then it just feels like slight engineering overkill to create a class with 2 members. Btw, I actually STILL sometimes prefer doing that over Pair because it serves as a descriptor for what the Pair actually is. But in this case, we are already returning an object that we built. And the totalCachedBytes just seems to me to be a description of the thing that we built. How many totalCachedBytes are in the TResultRowBuilder? So it seems off to me to put it in a second member of a Pair when it really describes something about the other member of the Pair of an object that we built. One more point: The decision of adding cachedBytes is in this new method that is creating the TResultRowBuilder. So the decision to add cached bytes has to be made somewhere. In your scenario, we keep the caller as/is totalCachedBytes += cachedBytes; // need to track some cachedBytes rowBuilder.addBytes(cachedBytes); // add these tracked bytes to the TResultRowBuilder I'm just saying keep the tracker inside the TResultRowBuilder from the caller perspective. The caller ALWAYS has to make the decision as to when to add Cached bytes...just through a call to TResultRowBuilder, as in: // totalCachedBytes += cachedBytes (line now commented out, not needed) rowBuilder.addCachedBytes(cachedBytes); // adding cachedBytes, decision by caller, totalCachedBytes inside the TResultRowBuilder But as I said...I'm not gonna hold up the code review for this. Just think it's a lot cleaner this way, based on how the code was written. 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 Ah, didn't realize that. They didn't have that "T" in front. I don't have a huge problem either way. While things are safer creating a new object, I don't always want to over-worry about breaking immutability when it seems sorta obvious. Of course here we are already creating an object and it's not really that bad either. It could have done with some rewriting perhaps, but I don't see anything super egregious here, so leaving as/is is fine by me. -- 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 13:18:10 +0000 Gerrit-HasComments: Yes
