Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 )
Change subject: IMPALA-9382: part 1: transposed profile prototype ...................................................................... Patch Set 15: (3 comments) Looked at the thrift changes, and it makes sense to me. I'm looking at the rest and I'll post comments once I make it through runtime-profile.h http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift File common/thrift/RuntimeProfile.thrift: http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift@62 PS15, Line 62: 3: required list<bool> has_value : 4: required list<i64> values It would be good to have explicit comments about the cardinality here and how the indices line up with the instances that are included. This applies in several places, so the comment may not be right here. If you wanted to back out the value for a specific instance, what would you do? i.e. the i'th index here corresponds to the i'th index of input_profiles, etc. http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift@142 PS15, Line 142: The first map key is the info string : // key. The second map key is a distinct value of that key. Just for clarity, maybe include an example key and example distinct value. http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift@144 PS15, Line 144: This means that the common case, where all Missing the rest of this sentence. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 15 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 12 Aug 2020 22:45:21 +0000 Gerrit-HasComments: Yes
