Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 )
Change subject: IMPALA-9382: part 1: transposed profile prototype ...................................................................... Patch Set 15: (4 comments) 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 h I added comments to the TAgg* structs to make it more explicit. http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift@113 PS15, Line 113: 3: required list<bool> has_value I'm realising that I'm a little inconsistent about naming these lists, and particularly about whether they're plural or not. I fixed the has_value/have_values inconsistency in favour of the former, cause it sounds less awkward to me. I didn't see anyhting else that would obviously improve clarity but lmk if you think I could clean any of this up. 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. This is a good point. I added a realistic example that motivates it. 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. Done -- 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 <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 13 Aug 2020 00:54:44 +0000 Gerrit-HasComments: Yes
