Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24153 )
Change subject: IMPALA-14597: Initial HBO support ...................................................................... Patch Set 17: Code-Review+1 (1 comment) Thanks for the code, love this feature! I put in one long optional comment with a small nit/question at the end. http://gerrit.cloudera.org:8080/#/c/24153/17/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/24153/17/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1947 PS17, Line 1947: populateHboThriftFields(msg); Heh, so this is actually the type of thing I was hoping to avoid with my last comment and it moved away from what I was trying to suggest rather than closer. :) I have a personal pet peeve having parameters filled in when called. I know this is more common practice in our C++ code, and, heh, I don't particularly like it there either, but I think we can avoid it here without any extra overhead. I'm gonna make this optional because I suppose this comes down to personal preference on this level. I'm still gonna +1 this code :) I mean, the method name is descriptive enough and the "toThrift()" method which mutates the "TPlanNode msg" parameter already violates my personal preference, and that change is out of scope here. But if it were me coding it? I would do 2 things different here: 1) Have one structure that contains both the "exec_stats" and the "Hbo_hash_keys" within the message. It is all HBO related, and imo, it slightly cleans up PlanNode which already has a ton of stuff going on with it. Maybe that's too many structures? Idk. 2) Have the populateHboThriftFields return this new structure. That way, the populateHboThriftFields becomes immutable rather than passing in the "msg". But a small nit with the current code: populateHboThriftFields returns a boolean. This is the only caller and it doesn't do anything with the return value. Did you mean to do something? Or should it be a void? -- To view, visit http://gerrit.cloudera.org:8080/24153 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ff60a8bd22c13c0ecad1198934cc96249b1015e Gerrit-Change-Number: 24153 Gerrit-PatchSet: 17 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Reviewer: Surya Hebbar <[email protected]> Gerrit-Comment-Date: Tue, 09 Jun 2026 15:22:45 +0000 Gerrit-HasComments: Yes
